Ephemeral Dust #30239

pull instagibbs wants to merge 9 commits into bitcoin:master from instagibbs:2024-03-general-ephemeral changing 20 files +1218 −3
  1. instagibbs commented at 5:31 pm on June 6, 2024: member

    A replacement for #29001

    Now that we have 1P1C relay, TRUC transactions and sibling eviction, it makes sense to retarget this feature more narrowly by not introducing a new output type, and simple focusing on the feature of allowing temporary dust in the mempool.

    Users of this can immediately use dust outputs as:

    1. Single keyed anchor (can be shared by multiple parties)
    2. Single unkeyed anchor, ala P2A

    Which is useful when the parent transaction cannot have fees for technical or accounting reasons.

    What I’m calling “keyed” anchors would be used anytime you don’t want a third party to be able to run off with the utxo. As a motivating example, in Ark there is the concept of a “forfeit transaction” which spends a “connector output”. The connector output would ideally be 0-value, but you would not want that utxo spend by anyone, because this would cause financial loss for the coordinator of the service: https://arkdev.info/docs/learn/concepts#forfeit-transaction

    Note that this specific use-case likely doesn’t work as it involves a tree of dust, but the connector idea in general demonstrates how it could be used.

    Another related example is connector outputs in BitVM2: https://bitvm.org/bitvm2.html .

    Note that non-TRUC usage will be impractical unless the minrelay requirement on individual transactions are dropped in general, which should happen post-cluster mempool.

    Lightning Network intends to use this feature post-29.0 if available: https://github.com/lightning/bolts/issues/1171#issuecomment-2373748582

    It’s also useful for Ark, ln-symmetry, spacechains, Timeout Trees, and other constructs with large presigned trees or other large-N party smart contracts.

  2. DrahtBot commented at 5:31 pm on June 6, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30239.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK glozow, theStack
    Concept NACK petertodd
    Concept ACK t-bast, murchandamus, stickies-v, hodlinator, tdb3
    Stale ACK sdaftuar

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31153 (bench: Remove various extraneous benchmarks by dergoegge)
    • #29247 (CAT in Tapscript (BIP-347) by 0xBEEFCAF3)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. instagibbs force-pushed on Jun 6, 2024
  4. DrahtBot added the label CI failed on Jun 6, 2024
  5. DrahtBot commented at 5:39 pm on June 6, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/25906194302

  6. instagibbs commented at 6:44 pm on June 6, 2024: member
    going to put this into draft until #29496 is merged, since dust checks are completely off when -acceptnonstdtxn=1. which is required for TRUC currently.
  7. instagibbs marked this as a draft on Jun 6, 2024
  8. instagibbs force-pushed on Jun 7, 2024
  9. instagibbs force-pushed on Jun 7, 2024
  10. instagibbs marked this as ready for review on Jun 7, 2024
  11. DrahtBot removed the label CI failed on Jun 8, 2024
  12. theStack commented at 5:22 pm on June 9, 2024: contributor
    Is it an option to be more restrictive and only allow zero-value outputs as ephemeral anchors, for not having to deal with the concept of dust at all? Or, asked differently: what would be the motivation/benefit for users to ever create an anchor output with nValue > 0? (Note that I haven’t looked too deep into the concept of ephemeral anchors and the predecessor PR #29001, so very likely I’m missing something obvious here, but I guess the answer could be interesting for other potential reviewers too.)
  13. instagibbs commented at 1:39 pm on June 10, 2024: member

    @theStack the primary motivation is to cover cases where non-0 value is attached to handle cases where a smart contract may want to “throw away” a few sats to fees, but otherwise cannot because of the 0-fee requirement of this PR for transactions with ephemeral anchors. If the ephemeral anchor-having transaction had non-0-fee, that would allow endogenous incentives to get it mined on its own, leaving the dust in the utxo set. As an example from the LN spec, [trimmed outputs(https://github.com/lightning/bolts/blob/master/03-transactions.md#trimmed-outputs) are directly added to the commitment transaction fee. Instead, spec writers could have the value flow to the ephemeral anchor, which is then spent to fees by the child transaction. Example spec here: https://github.com/instagibbs/bolts/commits/zero_fee_commitment

    It’s a fairly narrow motivation, and honestly I don’t love the timmed output to fees scheme, but also doesn’t make the code anymore complicated, so I think it’s worth consideration.

  14. DrahtBot added the label CI failed on Jun 16, 2024
  15. DrahtBot commented at 8:07 pm on June 16, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/25953908933

  16. bitcoin deleted a comment on Jun 16, 2024
  17. instagibbs force-pushed on Jun 17, 2024
  18. instagibbs commented at 2:54 pm on June 17, 2024: member
    rebased due to s/nVersion/version/ change for transactions causing a silent merge conflict
  19. DrahtBot removed the label CI failed on Jun 17, 2024
  20. instagibbs force-pushed on Jun 20, 2024
  21. DrahtBot added the label CI failed on Jul 2, 2024
  22. instagibbs renamed this:
    Ephemeral Anchors, take 2
    Ephemeral Dust
    on Jul 2, 2024
  23. DrahtBot added the label Needs rebase on Jul 2, 2024
  24. DrahtBot removed the label CI failed on Jul 3, 2024
  25. instagibbs force-pushed on Jul 8, 2024
  26. instagibbs commented at 8:30 pm on July 8, 2024: member
    rebased on master, and s/anchor/dust/ everywhere
  27. DrahtBot removed the label Needs rebase on Jul 8, 2024
  28. glozow commented at 10:08 am on July 15, 2024: member
    concept ACK
  29. petertodd commented at 2:40 pm on July 18, 2024: contributor

    Concept NACK

    There’s no reason to limit this behavior to TRUC/V3 transactions. The implementation should not ship with this restriction.

  30. instagibbs commented at 7:09 pm on July 18, 2024: member

    @petertodd I’ll give a non-TRUC version another try and report back. I may have been over-thinking things. If it works out it will be very similar in code structure…

    edit: oh of course I already noted in OP: We don’t allow individual transactions with 0-fee outside of TRUC, for now due to trimming computation concerns. Cluster mempool does away with this restriction entirely. I’ll rework the code to upgrade to non-TRUC usage once cluster mempool hits.

  31. petertodd commented at 1:09 am on July 19, 2024: contributor

    @petertodd I’ll give a non-TRUC version another try and report back. I may have been over-thinking things. If it works out it will be very similar in code structure…

    edit: oh of course I already noted in OP: We don’t allow individual transactions with 0-fee outside of TRUC, for now due to trimming computation concerns. Cluster mempool does away with this restriction entirely. I’ll rework the code to upgrade to non-TRUC usage once cluster mempool hits.

    I think the worthwhile question here is why are 0-fee CPFP’s required to signal TRUC/V3? I don’t see any reason why transaction creators should be forced to adhere to the restrictive TRUC/V3 rules if they do not wish to.

  32. t-bast commented at 9:29 am on July 19, 2024: contributor
    Concept ACK :+1:
  33. petertodd commented at 2:59 pm on July 19, 2024: contributor

    I’m also going to point out that restricting ephemeral dust transactions to zero fee seems to create protocol design problems around HTLCs: https://delvingbitcoin.org/t/ephemeral-anchors-and-mevil/383

    The simplest solution with dust HTLCs is to just put the value towards mining fees. Forcing them to be put towards the keyless ephemeral output potentially creates incentive problems around stealing that output. If ephemeral dust transactions have fewer rules around them, that kind of design problem goes away and protocols can do what makes sense for them without trying to meet TRUC/V3 requirements.

  34. t-bast commented at 3:50 pm on July 19, 2024: contributor

    Forcing them to be put towards the keyless ephemeral output potentially creates incentive problems around stealing that output.

    Not really, because it will always be the miner that will steal it (since they can replace any spend by a spend to themselves). As was detailed in the delving post you’re referencing, putting HTLC values in the keyless ephemeral output ensures that it’s spent and doesn’t pollute the utxo set, while putting the fees directly to the commitment transaction doesn’t incentivize spending that output?

  35. petertodd commented at 4:10 pm on July 19, 2024: contributor

    Stealing an ephemeral anchor output can only be done in certain conditions, at specific ranges of values. It is not always profitable due to overheads, and you can reduce those overheads by doing complex things.

    The game theory is much simpler if the dust HTLC funds go directly to fees.

    On July 19, 2024 5:50:49 PM GMT+02:00, Bastien Teinturier @.***> wrote:

    Forcing them to be put towards the keyless ephemeral output potentially creates incentive problems around stealing that output.

    Not really, because it will always be the miner that will steal it (since they can replace any spend by a spend to themselves). As was detailed in the delving post you’re referencing, putting HTLC values in the keyless ephemeral output ensures that it’s spent and doesn’t pollute the utxo set, while putting the fees directly to the commitment transaction doesn’t incentivize spending that output?

  36. instagibbs force-pushed on Jul 19, 2024
  37. instagibbs commented at 5:32 pm on July 19, 2024: member

    Ok, the rework ended up being quite easy.

    Pushed an update that generalizes to non-TRUC transactions, but as noted earlier, individual transactions have to meet minrelay for now unless they are TRUC, see/divert discussion here for that specific point: #26933 . If this restriction goes away, it will be usable(see tests I added with v2 transactions at the bottom where I checked it). Note that we are still constrained to 1P1C relay until we get general package relay.


    re:0-fee, yes it’s obviously sub-optimal for transaction creation for things like trimmed values. There’s no simple way that I can think of that ensures that dust isn’t mined. E.g., TxA with dust, followed by TxB spending the dust. Both enter mempool. Then TxB' replaces TxB, but doesn’t spend the dust output from TxA. If TxA has fees at or above minrelay, it may well be mined on its own.

    I don’t have any clean ways of doing that at present. If we figure it out, it’s another clean relaxation that should have easy logic to follow. @glozow had suggested long ago that we could allow any fee under minrelay for ephemeral tx, but this is still pretty limited in what amounts are allowed for fees. I’m not convinced it’s worth it vs thinking harder about figuring out how to allow unbounded feerates.

  38. t-bast commented at 8:25 am on July 22, 2024: contributor

    Stealing an ephemeral anchor output can only be done in certain conditions, at specific ranges of values. It is not always profitable due to overheads, and you can reduce those overheads by doing complex things.

    I don’t understand this hand-wavy comment, can you detail? I don’t see any complexity here. Whenever a miner would include a transaction spending an ephemeral output in a block:

    • if that ephemeral output is economical, the miner replaces the spending transaction by one that sends funds to themselves
    • otherwise, they keep the existing transaction (which in most cases will contain an unrelated input to bring fees)

    Sounds trivial enough?

  39. DrahtBot added the label CI failed on Jul 22, 2024
  40. DrahtBot commented at 4:54 pm on July 22, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27674254741

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  41. instagibbs force-pushed on Jul 22, 2024
  42. Mahmoud198425 approved
  43. in src/policy/ephemeral_policy.cpp:80 in 6eca593c83 outdated
    75+
    76+        // Process new dust
    77+        for (uint32_t i=0; i<tx->vout.size(); i++) {
    78+            if (IsDust(tx->vout[i], dust_relay_rate)) {
    79+                // CheckValidEphemeralTx should disallow multiples
    80+                Assume(map_tx_dust.count(tx->GetHash()) == 0);
    


    achow101 commented at 7:44 pm on July 22, 2024:

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

    nit: Use contains, here and elsewhere.

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

    instagibbs commented at 2:08 am on July 23, 2024:
    done
  44. in src/policy/ephemeral_policy.h:23 in 6eca593c83 outdated
    18+ * Otherwise it returns true.
    19+ */
    20+bool CheckValidEphemeralTx(const CTransaction& tx, CFeeRate dust_relay_fee, CAmount txfee, TxValidationState& state);
    21+
    22+/** Checks that all dust in package ends up spent by direct children. Assumes package is well-formed and sorted. */
    23+std::optional<uint256> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate);
    


    achow101 commented at 8:05 pm on July 22, 2024:

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

    Could you document what the return value is?


    instagibbs commented at 2:08 am on July 23, 2024:
    documented
  45. in src/policy/ephemeral_policy.cpp:59 in 6eca593c83 outdated
    68+            // We want to detect dangling dust too
    69+            unspent_dust.erase(tx_input.prevout);
    70+        }
    71+
    72+        if (!child_unspent_dust.empty()) {
    73+            return tx->GetHash();
    


    achow101 commented at 8:08 pm on July 22, 2024:

    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.


    instagibbs commented at 2:08 am on July 23, 2024:
    switched to txid since that’s how it’s being tracked
  46. in src/validation.cpp:1606 in 6eca593c83 outdated
    1599@@ -1560,6 +1600,19 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
    1600         }
    1601     }
    1602 
    1603+    // Run package-based dust spentness checks
    1604+    if (m_pool.m_opts.require_standard) {
    1605+        if (const auto ephemeral_violation{CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate)}) {
    1606+            const auto parent_wtxid = ephemeral_violation.value();
    


    achow101 commented at 8:09 pm on July 22, 2024:

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

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

    0            const auto parent_txid = ephemeral_violation.value();
    

    instagibbs commented at 2:08 am on July 23, 2024:
    done
  47. in src/policy/ephemeral_policy.cpp:64 in 6eca593c83 outdated
    65+                 child_unspent_dust.erase(prevout.hash);
    66+            }
    67+
    68+            // We want to detect dangling dust too
    69+            unspent_dust.erase(tx_input.prevout);
    70+        }
    


    achow101 commented at 8:17 pm on July 22, 2024:

    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.


    instagibbs commented at 8:58 pm on July 22, 2024:

    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.


    instagibbs commented at 9:12 pm on July 22, 2024:
    I can update the documentation reflecting this if you find the explanation compelling.

    achow101 commented at 9:23 pm on July 22, 2024:

    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.


    instagibbs commented at 2:08 am on July 23, 2024:
    added some documentation
  48. DrahtBot removed the label CI failed on Jul 22, 2024
  49. instagibbs force-pushed on Jul 23, 2024
  50. instagibbs force-pushed on Jul 23, 2024
  51. instagibbs force-pushed on Jul 24, 2024
  52. instagibbs commented at 8:29 pm on July 24, 2024: member
    added fuzz and unit tests which cover more than the functional tests can alone
  53. instagibbs force-pushed on Jul 24, 2024
  54. DrahtBot added the label CI failed on Jul 24, 2024
  55. DrahtBot commented at 8:32 pm on July 24, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27880415572

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  56. DrahtBot removed the label CI failed on Jul 25, 2024
  57. instagibbs force-pushed on Jul 26, 2024
  58. instagibbs force-pushed on Jul 27, 2024
  59. instagibbs force-pushed on Jul 29, 2024
  60. petertodd commented at 7:53 am on August 2, 2024: contributor

    It looks like we’ve created a new, rare, pinning vector here: in the event that a dust-generating transaction is reorged back into the mempool, transactions that otherwise could have spent an ordinary output of that transaction alone now can’t be accepted into the mempool because they fail the “spends all parent dust” check.

    For example, imagine a coinjoin is funded by a transaction A containing a dust output. After a reorg, the transaction A gets forced back into the mempool. Now the coinjoin (or double-spends of it) fails the spends all parents dust check, because it doesn’t spend the dust of A, and A may never get mined due to fee-rates increasing.

  61. instagibbs commented at 2:01 pm on August 2, 2024: member

    @petertodd I believe that’s incorrect, and looks like I wrote a test case for this: https://github.com/bitcoin/bitcoin/pull/30239/commits/63e097092253b25fa9d72af167d83d911c805c9c#diff-f3bb3623abbd14a7140e405175278c9187eb8c2cf6ff0279700ac507c5f4180cR334

    I show that a child spend of parent tx gets into mempool even if it has dust of its own, and fails to spend parent’s dust.

    Reorgs should hopefully be expensive, so we simply allow things back in mempool even if the spentness is violated.

    You can glance at the new usages of bypass_limits in the code changes to see. I should probably add a test showing that a tx with two dust or fee-having can’t use bypass_limits.

  62. instagibbs force-pushed on Aug 2, 2024
  63. glozow referenced this in commit 2aff9a36c3 on Aug 2, 2024
  64. instagibbs force-pushed on Aug 2, 2024
  65. instagibbs commented at 5:06 pm on August 2, 2024: member
    rebased on top of master to pick up PayToAnchor support for those who would like to test them together
  66. petertodd commented at 7:38 pm on August 2, 2024: contributor

    @petertodd I believe that’s incorrect, and looks like I wrote a test case for this: 63e0970#diff-f3bb3623abbd14a7140e405175278c9187eb8c2cf6ff0279700ac507c5f4180cR334

    I show that a child spend of parent tx gets into mempool even if it has dust of its own, and fails to spend parent’s dust.

    Reorgs should hopefully be expensive, so we simply allow things back in mempool even if the spentness is violated.

    You can glance at the new usages of bypass_limits in the code changes to see. I should probably add a test showing that a tx with two dust or fee-having can’t use bypass_limits.

    I think you’re misunderstanding the scenario I’m talking about. I’m not saying that transactions in a block won’t get back into your mempool when your node reorgs the block - they of course do get it due to the bypass_limits flag. What I’m saying is that subsequent transactions won’t be accepted, resulting in a pin.

    Concretely, imagine that the top block contains transaction A, which contains a dust output. Transaction B, which spends an output of A, can be accepted into your mempool as A is mined, and we can double-spend B as usual with a higher fee-rate.

    Now let’s reorg out the top block. Transaction A is forced into the mempool using bypass_limits; transaction B remains in the mempool. However, we can no longer double-spend B with a higher fee-rate, as any B2 is rejected on the basis that it does not spend the dust output of A. Thus, B is pinned by the existence of A in your mempool.

    While this is a relatively rare scenario, it’s not that rare. And there’s probably circumstances where attackers could take advantage of this opportunistically by waiting until they happened to have a transaction in the right state to use in an attack.

  67. petertodd commented at 7:51 pm on August 2, 2024: contributor
    I should point out that the TRUC code may suffer from this same category of problem around reorgs too…
  68. instagibbs commented at 8:06 pm on August 2, 2024: member

    Got it. Normally a reorg like this would result in an effective feerate reduction, as the entire package is now having to be paid for. With the dust spending restriction, it makes it impossible to replace later, exactly like a package limit pin.

    I should point out that the TRUC code may suffer from this same category of problem around reorgs too…

    Depends on the topo but yep, post-reorg package limits may be hit unexpectedly. Parent->child->grandchild, where parent was formerly confirmed, as an example. Regular chain limits are also possible to hit, f.e. if the re-entered parent was 100kvB, or full chain limits hit.

    Circling back to dust, I suppose bypass-ness of a transaction entry(the re-entered parent in this context) could be tracked and spenders could bypass the dust spentness check as well? Will mull it over.

  69. petertodd commented at 6:37 pm on August 3, 2024: contributor

    Got it. Normally a reorg like this would result in an effective feerate reduction, as the entire package is now having to be paid for. With the dust spending restriction, it makes it impossible to replace later, exactly like a package limit pin.

    Exactly. And unlike the inherent pin possible by the fact that the reorged transaction could simply be big, this variant works even if the transaction is small, making pulling it off potentially a lot easier.

    Personally I’d consider removing the restrictions on spending outputs of ephemeral dust transactions. For what Core is trying to accomplish here, I think it’s enough that the transaction was accepted into your mempool with the dust being spent. That’s enough of a nudge to get the vast majority of use-cases to do what we want them to do. It’s more important that this feature can be used securely by L2 implementations, without having to worry about security problems from complex edge cases.

    People who want to actually create dust intentionally can do so pretty easily anyway by just sending their transactions to one of the pools that doesn’t enforce the dust limit; most of the recent growth is probably due to intentional token protocols doing things for which rules like this are irrelevant.

    Suppose we do remove that restriction. You would be able to create an unspent dust UTXO by broadcasting a transaction package creating the UTXO in A, a transaction with a dust output and a non-dust output, and then spending it with a two input transaction B spending both those outputs. Then you double-spend B to remove the dust input while still providing miners an incentive to mine A.

    Thing is, to do this you have to spend about as much money in fees as it would have cost you in lost sats to create an unspent non-dust output instead. Probably more, because the dust limit is based on an unrealistically low 1sat/vb fee-rate. This isn’t actually any worse than the status quo for an attacker trying to create dust.

    Similarly, I don’t think we actually need the restriction only having one dust output per transaction. If they’ve all been spent to get the transaction into your mempool, playing games to get them unspent costs as much or more money as just creating non-dust UTXOs.

    Regular chain limits are also possible to hit, f.e. if the re-entered parent was 100kvB, or full chain limits hit.

    While I’m not sure off the top of my head if this is actually true, it should be the case that RBF’s of existing transactions should always be allowed even if package limits have been exceeded somehow. I proposed this principle awhile back as the “always replaceable invariant”: the mempool should always allow you to replace a transaction already in the mempool with a similar one at higher fee so that you can make forward progress.

    This specific issue contradicts that principal because it’s a hard restriction on the type of transaction allowed in a specific circumstance.

  70. hebasto added the label Needs CMake port on Aug 16, 2024
  71. instagibbs force-pushed on Aug 28, 2024
  72. instagibbs force-pushed on Aug 28, 2024
  73. hebasto removed the label Needs CMake port on Aug 29, 2024
  74. instagibbs force-pushed on Aug 30, 2024
  75. instagibbs force-pushed on Aug 30, 2024
  76. DrahtBot added the label Needs rebase on Sep 2, 2024
  77. instagibbs force-pushed on Sep 3, 2024
  78. DrahtBot removed the label Needs rebase on Sep 3, 2024
  79. instagibbs force-pushed on Sep 3, 2024
  80. in src/policy/ephemeral_policy.cpp:15 in 1baab61485 outdated
    10+    bool has_dust = false;
    11+    for (const CTxOut& txout : tx.vout) {
    12+        if (IsDust(txout, dust_relay_fee)) {
    13+            // We only allow a single dusty output
    14+            if (has_dust) {
    15+                return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust");
    


    glozow commented at 5:52 pm on September 3, 2024:
    0                return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx has more than 1 dust output");
    

    instagibbs commented at 3:17 pm on September 5, 2024:
    never actually reachable as this is already caught in IsStandardTx, removed this error condition
  81. in src/policy/ephemeral_policy.cpp:26 in 1baab61485 outdated
    21+    // No dust; it's complete standard already
    22+    if (!has_dust) return true;
    23+
    24+    // We never want to give incentives to mine this alone
    25+    if (txfee != 0) {
    26+        return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust");
    


    glozow commented at 5:52 pm on September 3, 2024:
    0        return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0 fee");
    

    instagibbs commented at 3:17 pm on September 5, 2024:
    done
  82. instagibbs commented at 5:57 pm on September 3, 2024: member

    Reworked the PR to relax IsStandardTx to allow a single dust output, and hopefully simplified the remaining checks that enforces spend of dusty parent’s dust. This should assure the same mempool invariants as before, but with more understandable and minimal changes to logic flow. @petertodd

    It’s more important that this feature can be used securely by L2 implementations, without having to worry about security problems from complex edge cases.

    I can’t speak for all wallets, but at least for CLN the fresh spend of funding outputs will be detected and the anchor spending will start anew. I think for first cut the tradeoff of having invariants that are easy to reason about is best. Given that there are actionable things wallets can do and the minimal complexity, I’m sticking with the overall current approach.

    While I’m not sure off the top of my head if this is actually true, it should be the case that RBF’s of existing transactions should always be allowed even if package limits have been exceeded somehow.

    Today’s mempool does a suboptimal job of dealing with chain limits and RBFs. Assuming cluster mempool, we still have the issue of what to do with cluster limits that are exceeded even after removing RBF conflicts, if any. This has been called “sibling eviction” and in general seems doable in a DoS-resistant way but is left to future work. It’s a problem I’m very interested in solving post-cluster mempool.

  83. instagibbs force-pushed on Sep 3, 2024
  84. DrahtBot commented at 8:46 pm on September 3, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29626041615

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  85. DrahtBot added the label CI failed on Sep 3, 2024
  86. instagibbs force-pushed on Sep 3, 2024
  87. DrahtBot removed the label CI failed on Sep 4, 2024
  88. petertodd commented at 11:39 am on September 4, 2024: contributor

    Today’s mempool does a suboptimal job of dealing with chain limits and RBFs. Assuming cluster mempool, we still have the issue of what to do with cluster limits that are exceeded even after removing RBF conflicts, if any. This has been called “sibling eviction” and in general seems doable in a DoS-resistant way but is left to future work. It’s a problem I’m very interested in solving post-cluster mempool.

    What I mean is, if you are doing an RBF replacement of one transaction with another transaction paying a higher fee-rate, with the same or smaller size, this should always be allowed even if somehow your mempool got into a state where a cluster limit was exceeded.

    Point is, the replacement isn’t making anything worse, so it should be allowed. Sibling eviction isn’t needed to support this case.

  89. in test/functional/mempool_ephemeral_dust.py:35 in c98fb6f490 outdated
    30+        if not expected:
    31+            expected = []
    32+        mempool = self.nodes[0].getrawmempool(verbose=False)
    33+        assert_equal(len(mempool), len(expected))
    34+        for tx in expected:
    35+            assert tx.rehash() in mempool
    


    glozow commented at 7:00 pm on September 4, 2024:
    could go in test_framework/mempool_util.py?

    instagibbs commented at 3:17 pm on September 5, 2024:
    done
  90. in test/functional/mempool_ephemeral_dust.py:177 in c98fb6f490 outdated
    172+        assert_equal(res["package_msg"], "transaction failed")
    173+        assert_equal(res["tx-results"][dusty_tx["wtxid"]]["error"], "dust")
    174+        assert_equal(self.nodes[0].getrawmempool(), [])
    175+
    176+    def test_nonzero_dust(self):
    177+        self.log.info("Test that a transaction with any value is standard as long as it's spent")
    


    glozow commented at 7:10 pm on September 4, 2024:
    0        self.log.info("Test that ephemeral dust is allowed for non-0 dust values")
    

    instagibbs commented at 3:17 pm on September 5, 2024:
    done
  91. in test/functional/mempool_ephemeral_dust.py:372 in c98fb6f490 outdated
    367+        # Re-connect and make sure we have same state still
    368+        self.connect_nodes(0, 1)
    369+        self.sync_all()
    370+
    371+    def test_acceptnonstd(self):
    372+        self.log.info("Test that dust is still allowed when acceptnonstd=1")
    


    glozow commented at 7:13 pm on September 4, 2024:
    acceptnonstdtxn

    instagibbs commented at 3:17 pm on September 5, 2024:
    done
  92. in test/functional/mempool_ephemeral_dust.py:95 in c98fb6f490 outdated
    90+
    91+        # But package evaluation succeeds
    92+        res = self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]])
    93+        assert_equal(res["package_msg"], "success")
    94+        assert_equal(len(self.nodes[0].getrawmempool()), 2)
    95+        self.assert_mempool_contents(expected=[dusty_tx["tx"], sweep_tx["tx"]])
    


    glozow commented at 7:14 pm on September 4, 2024:
    I think all of these assert_mempool_contents can also have a self.sync_mempools() to check that the ephemeral dust packages propagate.

    instagibbs commented at 3:17 pm on September 5, 2024:
    added an optional sync argument, only unused during reorg test
  93. in src/test/txvalidation_tests.cpp:133 in c98fb6f490 outdated
    128+    // We first start with nothing "in the mempool", using package checks
    129+
    130+    // Trivial single transaction with no dust
    131+    BOOST_CHECK(!CheckEphemeralSpends({dust_spend}, minrelay).has_value());
    132+
    133+    // Now with dust, ok because no spending transaction exists
    


    glozow commented at 7:28 pm on September 4, 2024:
    0    // Now with dust, ok because the tx has no dusty parents
    

    instagibbs commented at 3:17 pm on September 5, 2024:
    done
  94. in src/validation.cpp:1581 in c98fb6f490 outdated
    1574@@ -1560,6 +1575,19 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
    1575         }
    1576     }
    1577 
    1578+    // Run package-based dust spentness checks
    1579+    if (m_pool.m_opts.require_standard) {
    1580+        if (const auto ephemeral_violation{CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate)}) {
    1581+            const Txid parent_txid = ephemeral_violation.value();
    


    glozow commented at 7:29 pm on September 4, 2024:
    0            const Txid& parent_txid = ephemeral_violation.value();
    

    instagibbs commented at 3:17 pm on September 5, 2024:
    done
  95. in src/policy/policy.cpp:151 in c98fb6f490 outdated
    148+            num_dust_outputs++;
    149         }
    150     }
    151 
    152+    // Only one dust is permitted(on otherwise valid ephemeral dust)
    153+    if (num_dust_outputs > 1) {
    


    glozow commented at 7:40 pm on September 4, 2024:
    if you make 1 a constexpr MAX_DUST_OUTPUTS_PER_TX, you can static_assert that it == 1 as an assumption in CheckEphemeralSpends

    instagibbs commented at 3:18 pm on September 5, 2024:
    great idea, added
  96. in src/test/txvalidation_tests.cpp:160 in c98fb6f490 outdated
    155+    BOOST_CHECK(CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, dust_non_spend_both_parents}, minrelay).has_value());
    156+
    157+    auto dust_spend_both_parents = make_tx({COutPoint{dust_txid, dust_index}, COutPoint{dust_txid_2, dust_index}}, /*version=*/2);
    158+    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, dust_spend_both_parents}, minrelay).has_value());
    159+
    160+    // Spending all outputs is also correct
    


    glozow commented at 7:46 pm on September 4, 2024:
    0    // Spending other outputs is also correct, as long as the dusty one is spent
    

    instagibbs commented at 3:18 pm on September 5, 2024:
    done
  97. in src/policy/ephemeral_policy.cpp:48 in c98fb6f490 outdated
    43+
    44+    for (const auto& tx : package) {
    45+        std::unordered_set<Txid, SaltedTxidHasher> child_unspent_dust;
    46+        for (const auto& tx_input : tx->vin) {
    47+            // Parent tx had dust, child MUST be sweeping it
    48+            // if it's spending any output from parent
    


    glozow commented at 7:50 pm on September 4, 2024:
    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.

    instagibbs commented at 3:18 pm on September 5, 2024:
    elaborated a bit, let me know if it helps
  98. glozow commented at 7:56 pm on September 4, 2024: member

    Looks good. New rules seem easier to reason about (up to 1 as long as 0-fee, must spend all of my parent’s dust no matter what).

    Can you remind me of the use cases of a keyed dust output again? It seems slightly safer to require ephemeral dust be P2A, as it would allow anybody to clean them up in case something goes wrong, but perhaps too narrow.

    Additionally, could add a release note fragment.

  99. instagibbs force-pushed on Sep 5, 2024
  100. instagibbs commented at 3:40 pm on September 5, 2024: member

    With the new changes, I’m considering disallowing entry into the mempool if either the base fee or the modified fee is non-0. If miners really want to mine things generally with 0 fee that’s one thing, but offering an interface to avoid the dust cleanup seems suboptimal. @glozow

    Can you remind me of the use cases of a keyed dust output again?

    What I’m calling “keyed” anchors would be used anytime you don’t want a third party to be able to run off with the utxo. As a motivating example, in Ark there is the concept of a “forfeit transaction” which spends a “connector output”. The connector output would ideally be 0-value, but you would not want that utxo spend by anyone, because this would cause financial loss for the coordinator of the service: https://arkdev.info/docs/learn/concepts#forfeit-transaction

    Note that this specific use-case likely doesn’t work as it involves a tree of dust, but the connector idea in general demonstrates how it could be used.

    Another related example is connector outputs in BitVM2: https://bitvm.org/bitvm2.html . You could imag

    It seems slightly safer to require ephemeral dust be P2A, as it would allow anybody to clean them up in case something goes wrong, but perhaps too narrow.

    In general. if we have 0-value dust, I find it unlikely to hope that miners will clean it up for no reason?

    I’m also pretty hesitant to guess what people will find useful in general, e.g., if they’re worried about third party replacement cycling, they can be more cautious. The can also include a tapscript branch for anyonecanspend cleanup, just like LN does today!

    I feel moderately strongly about this, but of course relaxing policy further later is also another valid choice.

    Additionally, could add a release note fragment.

    Added

  101. glozow commented at 6:50 pm on September 5, 2024: member

    I’m also pretty hesitant to guess what people will find useful in general, e.g., if they’re worried about third party replacement cycling, they can be more cautious. The can also include a tapscript branch for anyonecanspend cleanup, just like LN does today!

    Seems reasonable to want keyed. Mostly wanted the rationale to be written up here.

  102. instagibbs force-pushed on Sep 5, 2024
  103. instagibbs commented at 10:13 pm on September 5, 2024: member
    @glozow copied some of the text to the OP
  104. DrahtBot added the label CI failed on Sep 6, 2024
  105. instagibbs commented at 1:41 pm on September 6, 2024: member
    CI failures seem like spurious timeouts of random tests
  106. instagibbs force-pushed on Sep 6, 2024
  107. instagibbs force-pushed on Sep 9, 2024
  108. instagibbs force-pushed on Sep 9, 2024
  109. instagibbs force-pushed on Sep 9, 2024
  110. DrahtBot removed the label CI failed on Sep 10, 2024
  111. petertodd commented at 1:15 pm on September 12, 2024: contributor

    In general. if we have 0-value dust, I find it unlikely to hope that miners will clean it up for no reason?

    Anyone can pay fees to clean up the dust. Doesn’t have to be miners specifically.

  112. in src/policy/ephemeral_policy.cpp:8 in 1332c4a077 outdated
    0@@ -0,0 +1,110 @@
    1+// Copyright (c) 2024-present The Bitcoin Core developers
    2+// Distributed under the MIT software license, see the accompanying
    3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
    4+
    5+#include<policy/ephemeral_policy.h>
    6+#include<policy/policy.h>
    7+
    8+bool CheckValidEphemeralTx(const CTransaction& tx, CFeeRate dust_relay_fee, CAmount txfee, TxValidationState& state)
    


    glozow commented at 8:24 pm on September 12, 2024:
    Any reason to use a CTransaction& instead of a CTransactionRef?

    instagibbs commented at 2:40 pm on September 13, 2024:
    nope, changed
  113. in src/policy/ephemeral_policy.h:15 in 1332c4a077 outdated
    10+#include <primitives/transaction.h>
    11+#include <txmempool.h>
    12+
    13+/** These utility functions ensure that ephemeral dust is safely
    14+ * created and spent without risking them entering the utxo
    15+ * set.
    


    glozow commented at 8:28 pm on September 12, 2024:
    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.

    instagibbs commented at 2:40 pm on September 13, 2024:
    added a hedge word
  114. in src/policy/ephemeral_policy.h:39 in 1332c4a077 outdated
    34+ * bringing fees, or itself being spent by another child, and so on.
    35+ */
    36+
    37+/** Does context-less checks about a single transaction.
    38+ * If it has relay dust, it returns false if tx has non-0 fee
    39+ * and sets relevant invalid state.
    


    glozow commented at 8:34 pm on September 12, 2024:
    0 * Returns false if the fee is non-zero and dust exists, populating state. True otherwise.
    

    instagibbs commented at 2:40 pm on September 13, 2024:
    taken
  115. in src/policy/ephemeral_policy.h:34 in 1332c4a077 outdated
    29+ * 1) None
    30+ * 2) TxA+TxB+TxC
    31+ * 3) TxA+TxC
    32+ * By requiring the child transaction to sweep any dust from the parent txn, we ensure that
    33+ * there is a single child only, and this child is the only transaction possible for
    34+ * bringing fees, or itself being spent by another child, and so on.
    


    glozow commented at 8:36 pm on September 12, 2024:
    I don’t really understand the “or itself being spent by another child, and so on” part

    instagibbs commented at 2:40 pm on September 13, 2024:
    clarified comment, let me know if that’s more clear
  116. in src/policy/ephemeral_policy.cpp:18 in 1332c4a077 outdated
    12+        for (const CTxOut& txout : tx.vout) {
    13+            if (IsDust(txout, dust_relay_fee)) {
    14+                return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0-fee");
    15+             }
    16+        }
    17+    }
    


    glozow commented at 8:37 pm on September 12, 2024:

    ?

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

    instagibbs commented at 2:40 pm on September 13, 2024:
    taken
  117. in src/policy/ephemeral_policy.h:56 in 1332c4a077 outdated
    48+std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate);
    49+
    50+/** Checks that individual transactions' parents have all their dust spent by this only-child transaction.
    51+ *  The function returns std::nullopt if all dust is properly spent or an error message string.
    52+ */
    53+std::optional<std::string> CheckEphemeralSpends(const CTransactionRef& ptx,
    


    glozow commented at 8:39 pm on September 12, 2024:
    Can you add documentation for when each function must be called, like we did for TRUC?

    instagibbs commented at 2:39 pm on September 13, 2024:
    added a sentence to each saying when they should be called
  118. in test/functional/mempool_ephemeral_dust.py:125 in 1332c4a077 outdated
    111+        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
    112+
    113+        # Node restart; doesn't allow allow ephemeral transaction back in due to individual submission
    114+        # resulting in 0-fee
    115+        self.restart_node(0)
    116+        self.restart_node(1)
    


    glozow commented at 8:57 pm on September 12, 2024:
    No need to restart node1?

    instagibbs commented at 2:39 pm on September 13, 2024:
    would cause the nodes to fail to sync their mempools, no? I find it easiest when the restarts are done in sync anyways.
  119. in test/functional/mempool_ephemeral_dust.py:121 in 1332c4a077 outdated
    108+        res = self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]])
    109+        assert_equal(res["package_msg"], "success")
    110+        assert_equal(len(self.nodes[0].getrawmempool()), 2)
    111+        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
    112+
    113+        # Node restart; doesn't allow allow ephemeral transaction back in due to individual submission
    


    glozow commented at 8:58 pm on September 12, 2024:
    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.

    instagibbs commented at 2:39 pm on September 13, 2024:
    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,
  120. in test/functional/mempool_ephemeral_dust.py:121 in 1332c4a077 outdated
    116+        self.restart_node(1)
    117+        self.connect_nodes(0, 1)
    118+        assert_mempool_contents(self, self.nodes[0], expected=[])
    119+
    120+    def test_fee_having_parent(self):
    121+        self.log.info("Test that a transaction with ephemeral dust may not have base fee, and ignores modified")
    


    glozow commented at 9:01 pm on September 12, 2024:
    nit: sentence structure kind of weird. the transaction ignores modified? I know what you mean, but maybe confusing wording

    instagibbs commented at 2:39 pm on September 13, 2024:
    dropped the reference to modified as it probably isn’t clarifying
  121. in test/functional/mempool_ephemeral_dust.py:128 in 1332c4a077 outdated
    123+        assert_equal(self.nodes[0].getrawmempool(), [])
    124+
    125+        sats_fee = 1
    126+        dusty_tx = self.wallet.create_self_transfer_multi(fee_per_output=sats_fee, version=3)
    127+        self.add_output_to_create_multi_result(dusty_tx)
    128+        assert_equal(dusty_tx["tx"].vout[0].nValue, 5000000000 - sats_fee) # main output is not dust
    


    glozow commented at 9:02 pm on September 12, 2024:

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

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

    instagibbs commented at 2:39 pm on September 13, 2024:
    added suggestion and a direct assertion that the main output amount is >330 the taproot dust amount
  122. in src/validation.cpp:940 in 1332c4a077 outdated
    934@@ -934,6 +935,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
    935                                     fSpendsCoinbase, nSigOpsCost, lock_points.value()));
    936     ws.m_vsize = entry->GetTxSize();
    937 
    938+    // Enforces 0 base fee for dust transactions, no incentive to be mined alone
    939+    if (m_pool.m_opts.require_standard) {
    940+        if (!CheckValidEphemeralTx(tx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, state)) {
    


    glozow commented at 9:07 pm on September 12, 2024:
    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.

    instagibbs commented at 9:45 pm on September 12, 2024:

    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?


    sdaftuar commented at 0:07 am on September 13, 2024:

    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.


    petertodd commented at 8:01 am on September 13, 2024:

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


    instagibbs commented at 3:55 pm on September 13, 2024:

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


    petertodd commented at 9:47 am on September 14, 2024:
    @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.
  123. in test/functional/mempool_ephemeral_dust.py:217 in 1332c4a077 outdated
    198+
    199+        self.generate(self.nodes[0], 1)
    200+        assert_mempool_contents(self, self.nodes[0], expected=[])
    201+
    202+    def test_non_truc(self):
    203+        self.log.info("Test that v2 dust-having transaction is rejected even if spent, because of min relay requirement")
    


    glozow commented at 9:11 pm on September 12, 2024:
    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

    instagibbs commented at 2:39 pm on September 13, 2024:
    dropped a note
  124. in test/functional/mempool_ephemeral_dust.py:412 in 1332c4a077 outdated
    407+
    408+        self.restart_node(0, extra_args=[])
    409+        self.restart_node(1, extra_args=[])
    410+        self.connect_nodes(0, 1)
    411+
    412+        assert_equal(self.nodes[0].getrawmempool(), [])
    


    glozow commented at 9:25 pm on September 12, 2024:
    Thoughts on adding this test before the changes in commit 1?

    instagibbs commented at 2:39 pm on September 13, 2024:
    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.
  125. in test/functional/mempool_ephemeral_dust.py:447 in 1332c4a077 outdated
    442+            dusty_txs.append(self.wallet.create_self_transfer_multi(fee_per_output=0, version=2))
    443+            self.add_output_to_create_multi_result(dusty_txs[-1])
    444+
    445+        all_parent_utxos = [utxo for tx in dusty_txs for utxo in tx["new_utxos"]]
    446+
    447+        # Missing one dust spend spend from a single parent, child rejected
    


    glozow commented at 9:29 pm on September 12, 2024:
    spend spend

    instagibbs commented at 2:39 pm on September 13, 2024:
    fixed
  126. glozow commented at 9:34 pm on September 12, 2024: member
    Finer comb of the first 2 commits
  127. instagibbs force-pushed on Sep 13, 2024
  128. instagibbs force-pushed on Oct 1, 2024
  129. in src/policy/ephemeral_policy.cpp:25 in dccb87df3e outdated
    20+{
    21+    // Package is topologically sorted, and PreChecks ensures that
    22+    // there is up to one dust output per tx.
    23+    static_assert(MAX_DUST_OUTPUTS_PER_TX == 1);
    24+
    25+    assert(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}));
    


    sdaftuar commented at 9:09 am on October 15, 2024:

    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.


    instagibbs commented at 4:03 pm on October 16, 2024:
    changed assert to Assume as requested, let it stumble along
  130. instagibbs commented at 10:13 am on October 15, 2024: member
    @fanquake can I get a 29.0 milestone for this?
  131. achow101 added this to the milestone 29.0 on Oct 15, 2024
  132. in src/policy/ephemeral_policy.cpp:19 in dccb87df3e outdated
    14+    }
    15+
    16+    return true;
    17+}
    18+
    19+std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate)
    


    sdaftuar commented at 10:43 am on October 15, 2024:

    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.

    instagibbs commented at 4:03 pm on October 16, 2024:
    done, put the new version in both AcceptMultipleTransactions and AcceptSingleTransaciton
  133. in src/validation.cpp:1074 in dccb87df3e outdated
    1069@@ -1062,6 +1070,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
    1070         }
    1071     }
    1072 
    1073+    // Ensure any parents in-mempool that have dust have it spent by this transaction
    1074+    if (!bypass_limits && m_pool.m_opts.require_standard) {
    


    sdaftuar commented at 10:58 am on October 15, 2024:

    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.


    instagibbs commented at 4:03 pm on October 16, 2024:

    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

  134. in test/functional/mempool_ephemeral_dust.py:67 in 2edef54ea9 outdated
    62+        self.test_unspent_ephemeral()
    63+        self.test_reorgs()
    64+        self.test_free_relay()
    65+
    66+    def test_normal_dust(self):
    67+        self.log.info("Create 0-value dusty output, show that it works inside truc when spend in package")
    


    sdaftuar commented at 9:12 am on October 16, 2024:
    nit: s/spend/spent/ ?

    instagibbs commented at 4:03 pm on October 16, 2024:
    fixed
  135. in test/functional/mempool_ephemeral_dust.py:191 in 2edef54ea9 outdated
    186+        assert_equal(res["package_msg"], "transaction failed")
    187+        assert_equal(res["tx-results"][dusty_tx["wtxid"]]["error"], "dust")
    188+        assert_equal(self.nodes[0].getrawmempool(), [])
    189+
    190+    def test_nonzero_dust(self):
    191+        self.log.info("Test that a single output of any size is allowed, not checking spending")
    


    sdaftuar commented at 9:27 am on October 16, 2024:
    Just want to make sure I understand this comment, does “any size” refer to the value in the output?

    instagibbs commented at 4:03 pm on October 16, 2024:
    yes, clarified
  136. in test/functional/mempool_ephemeral_dust.py:250 in 2edef54ea9 outdated
    245+        res = self.nodes[0].submitpackage([dusty_tx["hex"], unspent_sweep_tx["hex"]])
    246+        assert_equal(res["tx-results"][unspent_sweep_tx["wtxid"]]["error"], "ephemeral-dust-unspent, tx does not spend parent ephemeral dust")
    247+        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
    248+
    249+        # Spend works with dust spent
    250+        sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=dusty_tx["new_utxos"], version=3)
    


    sdaftuar commented at 9:35 am on October 16, 2024:
    Can we assert that this transaction is different from the one already in the mempool?

    instagibbs commented at 4:03 pm on October 16, 2024:
    done
  137. in test/functional/mempool_ephemeral_dust.py:244 in 2edef54ea9 outdated
    239+        sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=dusty_tx["new_utxos"], version=3)
    240+        self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]])
    241+        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
    242+
    243+        # Doesn't spend in-mempool dust output from parent
    244+        unspent_sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[dusty_tx["new_utxos"][0]], version=3)
    


    sdaftuar commented at 9:35 am on October 16, 2024:
    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.

    instagibbs commented at 4:03 pm on October 16, 2024:
    done
  138. in src/test/util/txmempool.cpp:144 in 83b0430885 outdated
    140@@ -141,6 +141,54 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns,
    141     return std::nullopt;
    142 }
    143 
    144+std::vector<uint32_t> GetDustIndexes(const CTransactionRef tx_ref, CFeeRate dust_relay_rate)
    


    sdaftuar commented at 10:27 am on October 16, 2024:
    Did you mean const CTransactionRef& tx_ref here?

    instagibbs commented at 4:03 pm on October 16, 2024:
    sure, changed
  139. sdaftuar commented at 10:42 am on October 16, 2024: member
    Read through the code (except for the fuzz test, which I plan to go back and review). Concept ACK.
  140. instagibbs force-pushed on Oct 16, 2024
  141. in src/policy/ephemeral_policy.cpp:8 in ad3e40ce53 outdated
    0@@ -0,0 +1,67 @@
    1+// Copyright (c) 2024-present The Bitcoin Core developers
    2+// Distributed under the MIT software license, see the accompanying
    3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
    4+
    5+#include<policy/ephemeral_policy.h>
    6+#include<policy/policy.h>
    7+
    8+bool CheckValidEphemeralTx(const CTransactionRef tx, CFeeRate dust_relay_fee, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
    


    sdaftuar commented at 4:02 pm on October 18, 2024:
    I think this should be const CTransactionRef& tx (and in the header file as well)

    instagibbs commented at 3:31 pm on October 21, 2024:
    done
  142. in src/test/fuzz/package_eval.cpp:332 in ad3e40ce53 outdated
    327+
    328+        const auto result_package = WITH_LOCK(::cs_main,
    329+                                    return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit, /*client_maxfeerate=*/{}));
    330+
    331+        const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, txs.back(), GetTime(),
    332+                                   /*bypass_limits=*/false, /*test_accept=*/!single_submit));
    


    sdaftuar commented at 4:05 pm on October 18, 2024:
    Perhaps we could let bypass_limits be filled in by the fuzzer, rather than hardcoded to false?

    instagibbs commented at 3:31 pm on October 21, 2024:
    done, previously this would have allowed non-dust-spending children and invalidate the invariant checks, now it shouldn’t matter
  143. in src/policy/ephemeral_policy.cpp:64 in ad3e40ce53 outdated
    48+                    if (IsDust(tx_output, dust_relay_rate)) {
    49+                        unspent_parent_dust.insert(COutPoint(parent_ref->GetHash(), out_index));
    50+                    }
    51+                }
    52+            }
    53+        }
    


    sdaftuar commented at 4:38 pm on October 18, 2024:

    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.


    instagibbs commented at 3:33 pm on October 21, 2024:

    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.


    instagibbs commented at 5:37 pm on October 21, 2024:
    @sdaftuar this should do it if you think something like this is reasonable: https://github.com/instagibbs/bitcoin/commit/6007146613fe663f0af30c24c47a19792f8e691d

    sdaftuar commented at 8:07 pm on October 21, 2024:
    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.

    instagibbs commented at 8:27 pm on October 21, 2024:

    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:

    0|               ns/op |                op/s |    err% |     total | benchmark
    1|--------------------:|--------------------:|--------:|----------:|:----------
    2|           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):

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

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


    sdaftuar commented at 3:47 pm on October 25, 2024:
    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).
  144. instagibbs force-pushed on Oct 21, 2024
  145. in src/rpc/mining.cpp:501 in 3923a13156 outdated
    497+    CTxMemPool& mempool = EnsureAnyMemPool(request.context);
    498+
    499+    // Non-0 fees are not allowed for entry, and modification not allowed afterwards
    500+    const auto& tx = mempool.get(hash);
    501+    if (tx && std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, mempool.m_opts.dust_relay_feerate); })) {
    502+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is not supported for transactions with ephemeral dust.");
    


    murchandamus commented at 6:11 pm on October 21, 2024:

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

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

    instagibbs commented at 8:07 pm on October 21, 2024:
    done
  146. in doc/release-notes-30239.md:5 in 7defd31351 outdated
    0@@ -0,0 +1,12 @@
    1+P2P and network changes
    2+-----------------------
    3+
    4+Ephemeral dust is a new concept that allows a single
    5+dust output in a tranasction, provided the transaction
    


    murchandamus commented at 6:17 pm on October 21, 2024:
    0dust output in a transaction, provided the transaction
    

    instagibbs commented at 8:07 pm on October 21, 2024:
    done
  147. murchandamus commented at 6:18 pm on October 21, 2024: contributor
    Concept ACK
  148. instagibbs force-pushed on Oct 21, 2024
  149. instagibbs force-pushed on Oct 21, 2024
  150. DrahtBot commented at 8:29 pm on October 21, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/31849856149

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  151. DrahtBot added the label CI failed on Oct 21, 2024
  152. DrahtBot removed the label CI failed on Oct 21, 2024
  153. in src/validation.cpp:1616 in fcbfed4630 outdated
    1609@@ -1592,6 +1610,19 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
    1610         return PackageMempoolAcceptResult(package_state, std::move(results));
    1611     }
    1612 
    1613+    // Now that we've bounded the resulting possible ancestry count, check parents for dust spends
    1614+    if (m_pool.m_opts.require_standard) {
    1615+        if (const auto ephemeral_violation{CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
    1616+            const Txid& parent_txid = ephemeral_violation.value();
    


    sdaftuar commented at 5:32 pm on October 25, 2024:
    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.

    instagibbs commented at 2:26 pm on October 26, 2024:
    out of date logging, updated
  154. in test/functional/mempool_ephemeral_dust.py:248 in d2ed713df9 outdated
    241+        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
    242+
    243+        # Doesn't spend in-mempool dust output from parent
    244+        unspent_sweep_tx = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=[dusty_tx["new_utxos"][0]], version=3)
    245+        assert_greater_than(unspent_sweep_tx["fee"], sweep_tx["fee"])
    246+        res = self.nodes[0].submitpackage([dusty_tx["hex"], unspent_sweep_tx["hex"]])
    


    sdaftuar commented at 7:11 pm on October 25, 2024:
    Is it also worth testing that this fails if submitted via sendrawtransaction as well, so we’re not just testing the AcceptPackage() code path?

    instagibbs commented at 2:26 pm on October 26, 2024:
    done
  155. in test/functional/mempool_ephemeral_dust.py:459 in d2ed713df9 outdated
    454+        # Cycle out the partial sweep to avoid triggering package RBF behavior which limits package to no in-mempool ancestors
    455+        cancel_sweep = self.wallet.create_self_transfer_multi(fee_per_output=21000, utxos_to_spend=[B_coin], version=2)
    456+        self.nodes[0].sendrawtransaction(cancel_sweep["hex"])
    457+        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"] for dusty_tx in dusty_txs] + [cancel_sweep["tx"]])
    458+
    459+        # Sweeps all dust, where most are already in-mempool
    


    sdaftuar commented at 8:35 pm on October 25, 2024:
    I think all the parents in-mempool at this point?

    instagibbs commented at 2:26 pm on October 26, 2024:
    correct, updated comment
  156. in src/test/fuzz/package_eval.cpp:166 in fecf9322b9 outdated
    161+}
    162+
    163+// Scan mempool for a tx that has spent dust, and return a "sibling"
    164+// spend of that dust to double-spend that dust-spend out of the mempool.
    165+// This assumes CheckMempoolEphemeralInvariants has passed for tx_pool.
    166+std::optional<COutPoint> GetEphemeralSibling(const CTxMemPool& tx_pool)
    


    sdaftuar commented at 9:10 pm on October 25, 2024:

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

    0flowchart TD
    1  TxA --> TxC
    2  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?


    instagibbs commented at 2:26 pm on October 26, 2024:
    yeah that’s confusing, reworded
  157. in src/test/fuzz/package_eval.cpp:224 in fecf9322b9 outdated
    220+        // Find something we may want to double-spend with two input single tx
    221+        std::optional<COutPoint> outpoint_to_rbf{GetEphemeralSibling(tx_pool)};
    222+        bool should_rbf_eph_spend = outpoint_to_rbf && fuzzed_data_provider.ConsumeBool();
    223+
    224+        // Make small packages
    225+        const auto num_txs = should_rbf_eph_spend ? 1 : (size_t) fuzzed_data_provider.ConsumeIntegralInRange<int>(1, 4);
    


    sdaftuar commented at 10:42 am on October 26, 2024:
    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.

    instagibbs commented at 2:26 pm on October 26, 2024:

    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.

  158. in src/test/txvalidation_tests.cpp:179 in 823d6be7ca outdated
    174+    auto child_with_dust = make_ephemeral_tx({COutPoint{parent_with_dust->GetHash(), dust_index}}, /*version=*/2);
    175+    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, parent_with_dust, child_with_dust}, minrelay, pool).has_value());
    176+
    177+    // Tests with parents in mempool
    178+
    179+    // Empty ancestors means this should never fail since it only checks parents' dust
    


    sdaftuar commented at 10:49 am on October 26, 2024:
    Comment needs to be updated I think?

    instagibbs commented at 2:26 pm on October 26, 2024:
    updated
  159. sdaftuar commented at 10:53 am on October 26, 2024: member
    Looks pretty good, I think most of my comments are nitty. Along those lines, you may want to retouch the commit message of fcbfed4630bdee6c52644c89e972ba7c130f47d5, as I think there’s a missing paren.
  160. instagibbs force-pushed on Oct 26, 2024
  161. sdaftuar commented at 8:10 pm on October 26, 2024: member
    Code review ACK e01bb3a06693efc77a173afdf25ed7ba631316a2
  162. DrahtBot requested review from murchandamus on Oct 26, 2024
  163. DrahtBot requested review from glozow on Oct 26, 2024
  164. DrahtBot added the label Needs rebase on Oct 29, 2024
  165. instagibbs force-pushed on Oct 29, 2024
  166. instagibbs commented at 8:23 pm on October 29, 2024: member
    rebased on master due to conflict
  167. DrahtBot removed the label Needs rebase on Oct 29, 2024
  168. in src/policy/ephemeral_policy.cpp:44 in 92d7ad2b13 outdated
    39+            if (processed_parent_set.contains(parent_txid)) continue;
    40+
    41+            // We look for an in-package or in-mempool dependency
    42+            CTransactionRef parent_ref = nullptr;
    43+            if (map_txid_ref.contains(parent_txid)) {
    44+                parent_ref = map_txid_ref[parent_txid];
    


    theStack commented at 11:06 pm on October 31, 2024:

    could avoid the duplicate map lookup:

    0            if (auto it = map_txid_ref.find(parent_txid); it != map_txid_ref.end()) {
    1                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))


    instagibbs commented at 1:56 pm on November 1, 2024:
    taken
  169. in doc/release-notes-30239.md:12 in 52b1acd16b outdated
     7+from this transaction, the spender must also spend
     8+this dust in addition to any other outputs.
     9+
    10+In other words, this type of transaction
    11+should be created in a transaction package where
    12+the dust is both created and spend simultaneously.
    


    theStack commented at 11:10 pm on October 31, 2024:
    0the dust is both created and spent simultaneously.
    

    instagibbs commented at 1:56 pm on November 1, 2024:
    done
  170. in test/functional/mempool_dust.py:84 in 191ca0506c outdated
    79+        assert_equal(self.nodes[0].getrawmempool(), [])
    80+
    81+        # Double dust, both unspent, with fees. Would have failed individual checks.
    82+        # Dust is 1 satoshi create_self_transfer_multi disallows 0
    83+        dusty_tx = self.wallet.create_self_transfer_multi(fee_per_output=1000, amount_per_output=1, num_outputs=2)
    84+        dust_txid = self.nodes[0].rpc.sendrawtransaction(hexstring=dusty_tx["hex"], maxfeerate=0)
    


    theStack commented at 11:12 pm on October 31, 2024:

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

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

    instagibbs commented at 1:56 pm on November 1, 2024:
    fixed
  171. in src/policy/ephemeral_policy.cpp:6 in 92d7ad2b13 outdated
    0@@ -0,0 +1,74 @@
    1+// Copyright (c) 2024-present The Bitcoin Core developers
    2+// Distributed under the MIT software license, see the accompanying
    3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
    4+
    5+#include<policy/ephemeral_policy.h>
    6+#include<policy/policy.h>
    


    theStack commented at 11:16 pm on October 31, 2024:

    whitespace nit

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

    instagibbs commented at 1:56 pm on November 1, 2024:
    fixed
  172. in src/policy/ephemeral_policy.cpp:12 in 92d7ad2b13 outdated
     7+
     8+bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_fee, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
     9+{
    10+    // We never want to give incentives to mine this transaction alone
    11+    if ((base_fee != 0 || mod_fee != 0) &&
    12+        std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_fee); })) {
    


    theStack commented at 11:17 pm on October 31, 2024:
    potential follow-up nit: could create a helper like HasDustOutputs (or IsDusty) and use that here and in the prioritisetransaction RPC

    instagibbs commented at 1:56 pm on November 1, 2024:
    Added HasDust to make it more obvious where these checks are being done
  173. in src/rpc/mining.cpp:496 in d75a77b11e outdated
    490@@ -491,7 +491,15 @@ static RPCHelpMan prioritisetransaction()
    491         throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is no longer supported, dummy argument to prioritisetransaction must be 0.");
    492     }
    493 
    494-    EnsureAnyMemPool(request.context).PrioritiseTransaction(hash, nAmount);
    495+    CTxMemPool& mempool = EnsureAnyMemPool(request.context);
    496+
    497+    // Non-0 fees are not allowed for entry, and modification not allowed afterwards
    


    theStack commented at 11:24 pm on October 31, 2024:
    0    // Non-0 fee dust transactions are not allowed for entry, and modification not allowed afterwards
    

    instagibbs commented at 1:56 pm on November 1, 2024:
    done
  174. in test/functional/test_framework/mempool_util.py:24 in b140ec1abd outdated
    20@@ -21,6 +21,20 @@
    21 
    22 ORPHAN_TX_EXPIRE_TIME = 1200
    23 
    24+def assert_mempool_contents(test_framework, node, expected=None, sync=True):
    


    theStack commented at 11:27 pm on October 31, 2024:
    follow-up idea: could take use of that helper in mempool_package_rbf.py (might be a nice “good first issue”)

    instagibbs commented at 1:56 pm on November 1, 2024:
    Deferring to Future Work

    hodlinator commented at 10:28 pm on November 6, 2024:
    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.
  175. in test/functional/mempool_ephemeral_dust.py:74 in b140ec1abd outdated
    69+        assert_equal(self.nodes[0].getrawmempool(), [])
    70+
    71+        dusty_tx = self.wallet.create_self_transfer_multi(fee_per_output=0, version=3)
    72+        self.add_output_to_create_multi_result(dusty_tx)
    73+
    74+        sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=dusty_tx["new_utxos"], version=3)
    


    theStack commented at 11:31 pm on October 31, 2024:
    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

    instagibbs commented at 1:56 pm on November 1, 2024:
    deferring to Future Work
  176. in test/functional/mempool_ephemeral_dust.py:90 in b140ec1abd outdated
    85+        self.nodes[0].prioritisetransaction(txid=dusty_tx["txid"], dummy=0, fee_delta=COIN)
    86+        test_res = self.nodes[0].testmempoolaccept([dusty_tx["hex"]])
    87+        assert not test_res[0]["allowed"]
    88+        assert_equal(test_res[0]["reject-reason"], "dust")
    89+        # Reset priority
    90+        self.nodes[0].prioritisetransaction(txid=dusty_tx["txid"], dummy=0, fee_delta=-COIN)
    


    theStack commented at 11:34 pm on October 31, 2024:
    nit: here and in other subtests, could verify that the modified fees are indeed zero again with an getprioritisedtransactions call after

    instagibbs commented at 1:56 pm on November 1, 2024:
    solid idea, done
  177. in test/functional/data/invalid_txs.py:255 in 92d7ad2b13 outdated
    251@@ -252,7 +252,7 @@ def get_tx(self):
    252         vin = self.valid_txin
    253         vin.scriptSig = CScript([opcode])
    254         tx.vin.append(vin)
    255-        tx.vout.append(CTxOut(1, basic_p2sh))
    256+        tx.vout.append(CTxOut(1000, basic_p2sh))
    


    theStack commented at 11:41 pm on October 31, 2024:
    is this change needed? seems to also pass with the dust value

    instagibbs commented at 1:56 pm on November 1, 2024:
    My guess is that previous implementation had a different validation ordering and was causing the case to fail in an unexpected way. Removed.
  178. theStack commented at 0:00 am on November 1, 2024: contributor

    Concept ACK

    Mostly looked and reasoned about the core commit 92d7ad2b135668cb7f6b0fd32be15fd6dd2f915c so far. From what I understand the “only one dust output” limit would not be strictly necessary and is far less important than the other two rules (must be 0-fee, child must spend all dust from the parent) in order to disisincentivize creating dust UTXOs, or am I missing something?

    What I left below are largely nits and follow-up ideas, feel free to ignore. Still want to look deeper into unit tests and the fuzzer.

  179. in test/functional/mempool_dust.py:74 in 191ca0506c outdated
    70@@ -71,9 +71,39 @@ def test_dust_output(self, node: TestNode, dust_relay_fee: Decimal,
    71         # finally send the transaction to avoid running out of MiniWallet UTXOs
    72         self.wallet.sendrawtransaction(from_node=node, tx_hex=tx_good_hex)
    73 
    74+    def test_dustrelay(self):
    


    glozow commented at 0:41 am on November 1, 2024:
    191ca0506ca69caa4d24c1f38b6f68742abb84dc nit: maybe test_dustrelayfee_zero or something
  180. instagibbs force-pushed on Nov 1, 2024
  181. sdaftuar commented at 6:41 pm on November 2, 2024: member
    ACK 51c2394eb8158113e73659f4ae65c237813bd5d2
  182. DrahtBot requested review from theStack on Nov 2, 2024
  183. in src/test/util/txmempool.cpp:165 in 6fa10b4537 outdated
    160+
    161+        std::vector<uint32_t> dust_indexes = GetDustIndexes(tx_info.tx, tx_pool.m_opts.dust_relay_feerate);
    162+
    163+        Assert(dust_indexes.size() < 2);
    164+
    165+        if (dust_indexes.empty()) return;
    


    theStack commented at 2:00 am on November 4, 2024:

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

    0        if (dust_indexes.empty()) continue;
    

    (same for the no-children condition below)


    instagibbs commented at 2:48 pm on November 4, 2024:
    Oof, this was making it a lot harder to hit the various cases! Fixed!
  184. DrahtBot requested review from theStack on Nov 4, 2024
  185. instagibbs force-pushed on Nov 4, 2024
  186. in src/policy/ephemeral_policy.h:28 in d5c564a24f outdated
    23+ * TxA, 0-fee with two outputs, one non-dust, one dust
    24+ * TxB, spends TxA's non-dust
    25+ * TxC, spends TxA's dust
    26+ *
    27+ * All the dust is spent if TxA+TxB+TxC is accepted, but the mining template may just pick
    28+ * up TxA+TxB rather than the three "legal configurations:
    


    theStack commented at 0:42 am on November 5, 2024:
    nit: missing closing double quote

    instagibbs commented at 2:09 pm on November 5, 2024:
    will fixup if I end up touching things
  187. in src/test/util/txmempool.cpp:144 in d73650d4ce outdated
    140@@ -141,7 +141,7 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns,
    141     return std::nullopt;
    142 }
    143 
    144-std::vector<uint32_t> GetDustIndexes(const CTransactionRef tx_ref, CFeeRate dust_relay_rate)
    145+std::vector<uint32_t> GetDustIndexes(const CTransactionRef& tx_ref, CFeeRate dust_relay_rate)
    


    theStack commented at 0:44 am on November 5, 2024:
    nit: should ideally be already part of the commit that introduces this function for minimal diff (here and in header)

    instagibbs commented at 2:09 pm on November 5, 2024:
    that’s a mistake, I’ll fix if I touch the PR
  188. theStack approved
  189. theStack commented at 0:55 am on November 5, 2024: contributor
    ACK 131bed19bdfc922328cad9781fa9122b6810a8fd
  190. DrahtBot requested review from sdaftuar on Nov 5, 2024
  191. in test/functional/mempool_dust.py:82 in 131bed19bd outdated
    77+        self.restart_node(0, extra_args=["-dustrelayfee=0"])
    78+
    79+        assert_equal(self.nodes[0].getrawmempool(), [])
    80+
    81+        # Double dust, both unspent, with fees. Would have failed individual checks.
    82+        # Dust is 1 satoshi create_self_transfer_multi disallows 0
    


    hodlinator commented at 9:47 am on November 5, 2024:

    nit: Missing word

    0        # Dust is 1 satoshi since create_self_transfer_multi disallows 0
    

    stickies-v commented at 12:21 pm on November 6, 2024:

    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

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

    instagibbs commented at 3:07 pm on November 12, 2024:
    will fixup in follwup
  192. in test/functional/mempool_dust.py:84 in 131bed19bd outdated
    79+        assert_equal(self.nodes[0].getrawmempool(), [])
    80+
    81+        # Double dust, both unspent, with fees. Would have failed individual checks.
    82+        # Dust is 1 satoshi create_self_transfer_multi disallows 0
    83+        dusty_tx = self.wallet.create_self_transfer_multi(fee_per_output=1000, amount_per_output=1, num_outputs=2)
    84+        dust_txid = self.nodes[0].sendrawtransaction(hexstring=dusty_tx["hex"], maxfeerate=0)
    


    hodlinator commented at 9:49 am on November 5, 2024:
    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.
  193. sdaftuar commented at 2:16 pm on November 5, 2024: member
    ACK 131bed19bdfc922328cad9781fa9122b6810a8fd
  194. sdaftuar approved
  195. in src/policy/ephemeral_policy.cpp:36 in d5c564a24f outdated
    31+    for (const auto& tx : package) {
    32+        map_txid_ref[tx->GetHash()] = tx;
    33+    }
    34+
    35+    for (const auto& tx : package) {
    36+        Txid txid = tx->GetHash();
    


    glozow commented at 9:07 pm on November 5, 2024:
    nit: const reference would be better

    hodlinator commented at 1:45 pm on November 6, 2024:

    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.


    instagibbs commented at 2:48 pm on November 12, 2024:
    I probably used it more than once before, will remove in follow-up
  196. in src/policy/ephemeral_policy.cpp:65 in d5c564a24f outdated
    60+                }
    61+            }
    62+
    63+            processed_parent_set.insert(parent_txid);
    64+        }
    65+
    


    glozow commented at 9:16 pm on November 5, 2024:
    Could speed up the common case a bit by checking unspent_parent_dust.empty() here, but perhaps not significantly
  197. in src/test/txvalidation_tests.cpp:117 in d73650d4ce outdated
    112+    CTxMemPool& pool = *Assert(m_node.mempool);
    113+    LOCK2(cs_main, pool.cs);
    114+    TestMemPoolEntryHelper entry;
    115+    CTxMemPool::setEntries empty_ancestors;
    116+
    117+    CFeeRate minrelay(1000);
    


    glozow commented at 9:40 pm on November 5, 2024:
    Question, why minrelay instead of DUST_RELAY_TX_FEE?
  198. in src/test/txvalidation_tests.cpp:153 in d73650d4ce outdated
    148+    const auto dust_txid_2 = grandparent_tx_2->GetHash();
    149+
    150+    // Spend dust from one but not another is ok, as long as second grandparent has no child
    151+    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, dust_spend}, minrelay, pool).has_value());
    152+
    153+    auto dust_non_spend_both_parents = make_tx({COutPoint{dust_txid, dust_index}, COutPoint{dust_txid_2, dust_index - 1}}, /*version=*/2);
    


    glozow commented at 9:49 pm on November 5, 2024:
    was this supposed to spend the non-dust from dust_txid 1?

    instagibbs commented at 2:03 pm on November 6, 2024:
    I think it’s correct, spending dust from first dusty tx but not second
  199. in src/test/txvalidation_tests.cpp:185 in d73650d4ce outdated
    180+    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1}, minrelay, pool).has_value());
    181+
    182+    // Add first grandparent to mempool and fetch entry
    183+    pool.addUnchecked(entry.FromTx(grandparent_tx_1));
    184+
    185+    // Ignores ancestors that aren't direct parents
    


    glozow commented at 9:54 pm on November 5, 2024:
    It also doesn’t know about ancestors, given that the parent isn’t given, right?

    instagibbs commented at 2:01 pm on November 6, 2024:
    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
  200. in src/policy/ephemeral_policy.cpp:8 in 131bed19bd outdated
    0@@ -0,0 +1,78 @@
    1+// Copyright (c) 2024-present The Bitcoin Core developers
    2+// Distributed under the MIT software license, see the accompanying
    3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
    4+
    5+#include <policy/ephemeral_policy.h>
    6+#include <policy/policy.h>
    7+
    8+bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
    


    rkrux commented at 9:45 am on November 6, 2024:
    Nit: This function is independent of the ephemeral nature and is generic enough to lie outside this file, near where IsDust() is.

    instagibbs commented at 2:25 pm on November 6, 2024:
    leaving as-is

    stickies-v commented at 12:45 pm on November 7, 2024:

    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

      0diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
      1index 6854822e35..6066d9b3ac 100644
      2--- a/src/policy/ephemeral_policy.cpp
      3+++ b/src/policy/ephemeral_policy.cpp
      4@@ -5,15 +5,10 @@
      5 #include <policy/ephemeral_policy.h>
      6 #include <policy/policy.h>
      7 
      8-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
      9-{
     10-    return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
     11-}
     12-
     13 bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
     14 {
     15     // We never want to give incentives to mine this transaction alone
     16-    if ((base_fee != 0 || mod_fee != 0) && HasDust(tx, dust_relay_rate)) {
     17+    if ((base_fee != 0 || mod_fee != 0) && !GetDust(*tx, dust_relay_rate).empty()) {
     18         return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0-fee");
     19     }
     20 
     21@@ -52,11 +47,8 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
     22 
     23             // Check for dust on parents
     24             if (parent_ref) {
     25-                for (uint32_t out_index = 0; out_index < parent_ref->vout.size(); out_index++) {
     26-                    const auto& tx_output = parent_ref->vout[out_index];
     27-                    if (IsDust(tx_output, dust_relay_rate)) {
     28-                        unspent_parent_dust.insert(COutPoint(parent_txid, out_index));
     29-                    }
     30+                for (const auto& out_index : GetDust(*parent_ref, dust_relay_rate)) {
     31+                    unspent_parent_dust.insert(COutPoint{parent_txid, out_index});
     32                 }
     33             }
     34 
     35diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
     36index 26140f9a02..98f40d38ff 100644
     37--- a/src/policy/ephemeral_policy.h
     38+++ b/src/policy/ephemeral_policy.h
     39@@ -34,9 +34,6 @@
     40  * are the only way to bring fees.
     41  */
     42 
     43-/** Returns true if transaction contains dust */
     44-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate);
     45-
     46 /* All the following checks are only called if standardness rules are being applied. */
     47 
     48 /** Must be called for each transaction once transaction fees are known.
     49diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
     50index 21c35af5cc..ed33692823 100644
     51--- a/src/policy/policy.cpp
     52+++ b/src/policy/policy.cpp
     53@@ -67,6 +67,15 @@ bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFeeIn)
     54     return (txout.nValue < GetDustThreshold(txout, dustRelayFeeIn));
     55 }
     56 
     57+std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate)
     58+{
     59+    std::vector<uint32_t> dust_outputs;
     60+    for (uint32_t i{0}; i < tx.vout.size(); ++i) {
     61+        if (IsDust(tx.vout[i], dust_relay_rate)) dust_outputs.push_back(i);
     62+    }
     63+    return dust_outputs;
     64+}
     65+
     66 bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_datacarrier_bytes, TxoutType& whichType)
     67 {
     68     std::vector<std::vector<unsigned char> > vSolutions;
     69@@ -129,7 +138,6 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
     70     }
     71 
     72     unsigned int nDataOut = 0;
     73-    unsigned int num_dust_outputs{0};
     74     TxoutType whichType;
     75     for (const CTxOut& txout : tx.vout) {
     76         if (!::IsStandard(txout.scriptPubKey, max_datacarrier_bytes, whichType)) {
     77@@ -142,13 +150,11 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
     78         else if ((whichType == TxoutType::MULTISIG) && (!permit_bare_multisig)) {
     79             reason = "bare-multisig";
     80             return false;
     81-        } else if (IsDust(txout, dust_relay_fee)) {
     82-            num_dust_outputs++;
     83         }
     84     }
     85 
     86     // Only MAX_DUST_OUTPUTS_PER_TX dust is permitted(on otherwise valid ephemeral dust)
     87-    if (num_dust_outputs > MAX_DUST_OUTPUTS_PER_TX) {
     88+    if (GetDust(tx, dust_relay_fee).size() > MAX_DUST_OUTPUTS_PER_TX) {
     89         reason = "dust";
     90         return false;
     91     }
     92diff --git a/src/policy/policy.h b/src/policy/policy.h
     93index 0488f8dbee..9e36d3f610 100644
     94--- a/src/policy/policy.h
     95+++ b/src/policy/policy.h
     96@@ -129,6 +129,9 @@ CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFee);
     97 
     98 bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFee);
     99 
    100+/** Get the vout index numbers of all dust outputs */
    101+std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate);
    102+
    103 bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_datacarrier_bytes, TxoutType& whichType);
    104 
    105 
    

    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.


    instagibbs commented at 2:52 pm on November 12, 2024:
    will handle on follow-up
  201. in src/policy/ephemeral_policy.cpp:21 in 131bed19bd outdated
    16+    if ((base_fee != 0 || mod_fee != 0) && HasDust(tx, dust_relay_rate)) {
    17+        return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0-fee");
    18+    }
    19+
    20+    return true;
    21+}
    


    rkrux commented at 9:48 am on November 6, 2024:

    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.


    instagibbs commented at 2:28 pm on November 6, 2024:

    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.


    hodlinator commented at 8:18 pm on November 6, 2024:
    Could rename it PreCheckValidEphemeralTx to signify that it doesn’t perform full validation.

    instagibbs commented at 2:49 pm on November 12, 2024:
    good idea, will rename in follow-up
  202. in src/policy/ephemeral_policy.cpp:59 in 131bed19bd outdated
    54+            if (parent_ref) {
    55+                for (uint32_t out_index = 0; out_index < parent_ref->vout.size(); out_index++) {
    56+                    const auto& tx_output = parent_ref->vout[out_index];
    57+                    if (IsDust(tx_output, dust_relay_rate)) {
    58+                        unspent_parent_dust.insert(COutPoint(parent_txid, out_index));
    59+                    }
    


    rkrux commented at 9:52 am on November 6, 2024:
    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?

    instagibbs commented at 2:29 pm on November 6, 2024:
    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.
  203. in src/policy/ephemeral_policy.cpp:23 in 131bed19bd outdated
    18+    }
    19+
    20+    return true;
    21+}
    22+
    23+std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
    


    rkrux commented at 10:06 am on November 6, 2024:
    Overall this is a pretty neat function that I enjoyed going through!
  204. in src/policy/policy.cpp:154 in 131bed19bd outdated
    151 
    152+    // Only MAX_DUST_OUTPUTS_PER_TX dust is permitted(on otherwise valid ephemeral dust)
    153+    if (num_dust_outputs > MAX_DUST_OUTPUTS_PER_TX) {
    154+        reason = "dust";
    155+        return false;
    156+    }
    


    rkrux commented at 10:35 am on November 6, 2024:
    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.

    instagibbs commented at 2:30 pm on November 6, 2024:
    yes, IsStandardTx check is very early in PreChecks, not requiring access to any utxo information. including fees.
  205. in src/policy/policy.h:83 in 131bed19bd outdated
    76@@ -77,6 +77,10 @@ static const unsigned int MAX_OP_RETURN_RELAY = 83;
    77  */
    78 static constexpr unsigned int EXTRA_DESCENDANT_TX_SIZE_LIMIT{10000};
    79 
    80+/**
    81+ * Maximum number of ephemeral dust outputs allowed.
    82+ */
    83+static constexpr unsigned int MAX_DUST_OUTPUTS_PER_TX{1};
    


    rkrux commented at 10:37 am on November 6, 2024:
    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().

    instagibbs commented at 2:32 pm on November 6, 2024:
    Can change to MAX_EPHEMERAL_DUST_OUTPUTS_PER_TX if I touch things
  206. in src/rpc/mining.cpp:500 in 131bed19bd outdated
    496+    CTxMemPool& mempool = EnsureAnyMemPool(request.context);
    497+
    498+    // Non-0 fee dust transactions are not allowed for entry, and modification not allowed afterwards
    499+    const auto& tx = mempool.get(hash);
    500+    if (tx && HasDust(tx, mempool.m_opts.dust_relay_feerate)) {
    501+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is not supported for transactions with dust outputs.");
    


    rkrux commented at 10:38 am on November 6, 2024:

    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.


    glozow commented at 1:18 pm on November 6, 2024:
    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.

    instagibbs commented at 2:14 pm on November 6, 2024:
    alternatively could just check mempool.m_opts.require_standard as well?

    instagibbs commented at 2:33 pm on November 6, 2024:
    This is as intended. We don’t allow priority on anything with dust, no matter how many non-zero dust outputs.

    glozow commented at 12:05 pm on November 12, 2024:
    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

    instagibbs commented at 3:27 pm on November 12, 2024:
    will add in follow-up
  207. in src/policy/ephemeral_policy.cpp:41 in 131bed19bd outdated
    36+        Txid txid = tx->GetHash();
    37+        std::unordered_set<Txid, SaltedTxidHasher> processed_parent_set;
    38+        std::unordered_set<COutPoint, SaltedOutpointHasher> unspent_parent_dust;
    39+
    40+        for (const auto& tx_input : tx->vin) {
    41+            const Txid& parent_txid{tx_input.prevout.hash};
    


    rkrux commented at 11:59 am on November 6, 2024:

    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.


    rkrux commented at 12:02 pm on November 6, 2024:
    Aah nvm, IsDust works on TxOuts.
  208. rkrux commented at 12:00 pm on November 6, 2024: none

    Successful make and functional tests at 131bed19bdfc922328cad9781fa9122b6810a8fd.

    I’ve gone through only the src/policy/, src/rpc/, src/validation.cpp yet. My comments are mostly around code structure. I plan to review the functional tests and src/tests/* very soon.

  209. in test/functional/mempool_ephemeral_dust.py:467 in 131bed19bd outdated
    462+        # Sweeps all dust, where all dusty txs are already in-mempool
    463+        sweep_tx = self.wallet.create_self_transfer_multi(fee_per_output=25000, utxos_to_spend=all_parent_utxos, version=2)
    464+
    465+        res = self.nodes[0].submitpackage([dusty_tx["hex"] for dusty_tx in dusty_txs] + [sweep_tx["hex"]])
    466+        assert_equal(res['package_msg'], "success")
    467+        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"] for dusty_tx in dusty_txs] + [sweep_tx["tx"], cancel_sweep["tx"]])
    


    glozow commented at 12:57 pm on November 6, 2024:
    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.

    instagibbs commented at 2:09 pm on November 6, 2024:
    could be worth a comment
  210. in test/functional/mempool_ephemeral_dust.py:473 in 131bed19bd outdated
    468+
    469+        self.generate(self.nodes[0], 25)
    470+        self.wallet.rescan_utxos()
    471+        assert_equal(self.nodes[0].getrawmempool(), [])
    472+
    473+        # Other topology tests require relaxation of submitpackage topology
    


    glozow commented at 1:12 pm on November 6, 2024:
    I think “other topology” is a little under-specified here, could be helpful to name what tests you think could be added
  211. in test/functional/mempool_ephemeral_dust.py:404 in 131bed19bd outdated
    399+        # Re-connect and make sure we have same state still
    400+        self.connect_nodes(0, 1)
    401+        self.sync_all()
    402+
    403+    # N.B. this extra_args can be removed post cluster mempool
    404+    def test_free_relay(self):
    


    glozow commented at 1:13 pm on November 6, 2024:
    nit: test_no_minrelay_fee could be better
  212. glozow commented at 1:21 pm on November 6, 2024: member

    code review ack. I still need to review tests more thoroughly. None of these comments are blocking.

    Trying to summarize why these would propagate via 1p1c package relay, please correct me if I’m wrong:

    • Essentially there is no longer anything “wrong” with a transaction that has 1 dust output in it since IsStandardTx has been changed. It’s not TX_NOT_STANDARD. It needs to be 0 fee to pass CheckValidEphemeralTx, and then it should fail on feerate for TX_RECONSIDERABLE. That’s why the parent can be picked up by the package relay logic. Child also needs to pass CheckEphemeralSpends, but otherwise neatly fits into 1p1c.
    • It’s nice that CheckValidEphemeralTx comes before TX_RECONSIDERABLE, so we don’t waste 1p1c cycles. But it’s not strictly necessary to ensure an ephemeral tx is eligible for reconsideration.
  213. DrahtBot requested review from glozow on Nov 6, 2024
  214. in src/policy/ephemeral_policy.cpp:43 in 131bed19bd outdated
    38+        std::unordered_set<COutPoint, SaltedOutpointHasher> unspent_parent_dust;
    39+
    40+        for (const auto& tx_input : tx->vin) {
    41+            const Txid& parent_txid{tx_input.prevout.hash};
    42+            // Skip parents we've already checked dust for
    43+            if (processed_parent_set.contains(parent_txid)) continue;
    


    hodlinator commented at 1:56 pm on November 6, 2024:

    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.

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

    instagibbs commented at 2:46 pm on November 12, 2024:
    might be heretical but I find as-is easier to read? Can add to follow-up if demand is there.
  215. in src/rpc/mining.cpp:501 in 131bed19bd outdated
    497+
    498+    // Non-0 fee dust transactions are not allowed for entry, and modification not allowed afterwards
    499+    const auto& tx = mempool.get(hash);
    500+    if (tx && HasDust(tx, mempool.m_opts.dust_relay_feerate)) {
    501+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is not supported for transactions with dust outputs.");
    502+    }
    


    hodlinator commented at 2:22 pm on November 6, 2024:

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


    stickies-v commented at 6:34 pm on November 8, 2024:

    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?


    instagibbs commented at 3:26 pm on November 12, 2024:
    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.

    hodlinator commented at 1:18 pm on November 13, 2024:

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


    instagibbs commented at 1:59 pm on November 13, 2024:

    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?


    hodlinator commented at 11:29 pm on November 13, 2024:

    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.

  216. in test/functional/test_framework/mempool_util.py:36 in 131bed19bd outdated
    31+    if not expected:
    32+        expected = []
    33+    mempool = node.getrawmempool(verbose=False)
    34+    assert_equal(len(mempool), len(expected))
    35+    for tx in expected:
    36+        assert tx.rehash() in mempool
    


    hodlinator commented at 2:31 pm on November 6, 2024:
    There’s an edge-case where expected accidentally contains duplicate tx instances (since it’s a list, not a set), meaning they might exist in the mempool, and the lengths may be equal, but the mempool may contain some other txs.
  217. in src/policy/ephemeral_policy.cpp:5 in 131bed19bd outdated
    0@@ -0,0 +1,78 @@
    1+// Copyright (c) 2024-present The Bitcoin Core developers
    2+// Distributed under the MIT software license, see the accompanying
    3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
    4+
    5+#include <policy/ephemeral_policy.h>
    


    stickies-v commented at 3:08 pm on November 6, 2024:

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

     0diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
     1index 6854822e35..3a9a3fa530 100644
     2--- a/src/policy/ephemeral_policy.cpp
     3+++ b/src/policy/ephemeral_policy.cpp
     4@@ -5,6 +5,22 @@
     5 #include <policy/ephemeral_policy.h>
     6 #include <policy/policy.h>
     7 
     8+#include <consensus/validation.h>
     9+#include <policy/feerate.h>
    10+#include <policy/packages.h>
    11+#include <primitives/transaction.h>
    12+#include <txmempool.h>
    13+#include <util/check.h>
    14+#include <util/hasher.h>
    15+
    16+#include <algorithm>
    17+#include <cstdint>
    18+#include <map>
    19+#include <memory>
    20+#include <unordered_set>
    21+#include <utility>
    22+#include <vector>
    23+
    24 bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
    25 {
    26     return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
    27diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
    28index 26140f9a02..742cb30280 100644
    29--- a/src/policy/ephemeral_policy.h
    30+++ b/src/policy/ephemeral_policy.h
    31@@ -5,10 +5,15 @@
    32 #ifndef BITCOIN_POLICY_EPHEMERAL_POLICY_H
    33 #define BITCOIN_POLICY_EPHEMERAL_POLICY_H
    34 
    35+#include <consensus/amount.h>
    36 #include <policy/packages.h>
    37-#include <policy/policy.h>
    38 #include <primitives/transaction.h>
    39-#include <txmempool.h>
    40+
    41+#include <optional>
    42+
    43+class CFeeRate;
    44+class CTxMemPool;
    45+class TxValidationState;
    46 
    47 /** These utility functions ensure that ephemeral dust is safely
    48  * created and spent without unduly risking them entering the utxo
    

    instagibbs commented at 2:52 pm on November 12, 2024:
    will touch in follow-up
  218. in src/policy/ephemeral_policy.cpp:10 in d5c564a24f outdated
     5+#include <policy/ephemeral_policy.h>
     6+#include <policy/policy.h>
     7+
     8+bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
     9+{
    10+    return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
    


    stickies-v commented at 3:29 pm on November 6, 2024:

    In d5c564a24f96ec384b2de68bf87e6eb3ad9239d2:

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

     0diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
     1index 6854822e35..569f9a95b5 100644
     2--- a/src/policy/ephemeral_policy.cpp
     3+++ b/src/policy/ephemeral_policy.cpp
     4@@ -7,7 +7,7 @@
     5 
     6 bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
     7 {
     8-    return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
     9+    return std::ranges::any_of(tx->vout, [&](const auto& output) { return IsDust(output, dust_relay_rate); });
    10 }
    11 
    12 bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
    13@@ -22,7 +22,7 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
    14 
    15 std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
    16 {
    17-    if (!Assume(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}))) {
    18+    if (!Assume(std::ranges::all_of(package, [](const auto& tx){return tx != nullptr;}))) {
    19         // Bail out of spend checks if caller gave us an invalid package
    20         return std::nullopt;
    21     }
    

    instagibbs commented at 2:51 pm on November 12, 2024:
    will touch on followup
  219. in src/policy/ephemeral_policy.h:53 in d5c564a24f outdated
    48+/** Must be called for each transaction(package) if any dust is in the package.
    49+ *  Checks that each transaction's parents have their dust spent by the child,
    50+ *  where parents are either in the mempool or in the package itself.
    51+ *  The function returns std::nullopt if all dust is properly spent, or the txid of the violating child spend.
    52+ */
    53+std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
    


    stickies-v commented at 4:59 pm on November 6, 2024:

    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:

     0diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
     1index 6854822e35..10a8ab1938 100644
     2--- a/src/policy/ephemeral_policy.cpp
     3+++ b/src/policy/ephemeral_policy.cpp
     4@@ -5,6 +5,8 @@
     5 #include <policy/ephemeral_policy.h>
     6 #include <policy/policy.h>
     7 
     8+#include <utility>
     9+
    10 bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
    11 {
    12     return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
    13@@ -20,11 +22,11 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
    14     return true;
    15 }
    16 
    17-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
    18+std::pair<bool, std::optional<Txid>> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
    19 {
    20     if (!Assume(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}))) {
    21         // Bail out of spend checks if caller gave us an invalid package
    22-        return std::nullopt;
    23+        return {true, std::nullopt};
    24     }
    25 
    26     std::map<Txid, CTransactionRef> map_txid_ref;
    27@@ -70,9 +72,9 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
    28         }
    29 
    30         if (!unspent_parent_dust.empty()) {
    31-            return txid;
    32+            return {false, txid};
    33         }
    34     }
    35 
    36-    return std::nullopt;
    37+    return {true, std::nullopt};
    38 }
    39diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
    40index 26140f9a02..7235dd7500 100644
    41--- a/src/policy/ephemeral_policy.h
    42+++ b/src/policy/ephemeral_policy.h
    43@@ -10,6 +10,8 @@
    44 #include <primitives/transaction.h>
    45 #include <txmempool.h>
    46 
    47+#include <utility>
    48+
    49 /** These utility functions ensure that ephemeral dust is safely
    50  * created and spent without unduly risking them entering the utxo
    51  * set.
    52@@ -50,6 +52,6 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
    53  *  where parents are either in the mempool or in the package itself.
    54  *  The function returns std::nullopt if all dust is properly spent, or the txid of the violating child spend.
    55  */
    56-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
    57+std::pair<bool, std::optional<Txid>> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
    58 
    59 #endif // BITCOIN_POLICY_EPHEMERAL_POLICY_H
    60diff --git a/src/validation.cpp b/src/validation.cpp
    61index 2f3e7d61a8..94cbdf2766 100644
    62--- a/src/validation.cpp
    63+++ b/src/validation.cpp
    64@@ -1456,11 +1456,11 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
    65     }
    66 
    67     if (m_pool.m_opts.require_standard) {
    68-        if (const auto ephemeral_violation{CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
    69-            const Txid& txid = ephemeral_violation.value();
    70-            Assume(txid == ptx->GetHash());
    71+        const auto& [is_ephemeral, offending_txid] = CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool);
    72+        if (!is_ephemeral) {
    73+            Assume(offending_txid.value() == ptx->GetHash());
    74             ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
    75-                          strprintf("tx %s did not spend parent's ephemeral dust", txid.ToString()));
    76+                          strprintf("tx %s did not spend parent's ephemeral dust", offending_txid.value().ToString()));
    77             return MempoolAcceptResult::Failure(ws.m_state);
    78         }
    79     }
    80@@ -1605,13 +1605,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
    81 
    82     // Now that we've bounded the resulting possible ancestry count, check package for dust spends
    83     if (m_pool.m_opts.require_standard) {
    84-        if (const auto ephemeral_violation{CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
    85-            const Txid& child_txid = ephemeral_violation.value();
    86+        const auto& [is_ephemeral, offending_txid] = CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool);
    87+        if (!is_ephemeral) {
    88             TxValidationState child_state;
    89             child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
    90-                          strprintf("tx %s did not spend parent's ephemeral dust", child_txid.ToString()));
    91+                          strprintf("tx %s did not spend parent's ephemeral dust", offending_txid.value().ToString()));
    92             package_state.Invalid(PackageValidationResult::PCKG_TX, "unspent-dust");
    93-            results.emplace(child_txid, MempoolAcceptResult::Failure(child_state));
    94+            results.emplace(offending_txid.value(), MempoolAcceptResult::Failure(child_state));
    95             return PackageMempoolAcceptResult(package_state, std::move(results));
    96         }
    97     }
    

    stickies-v commented at 4:49 pm on November 8, 2024:

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

     0diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
     1index 6854822e35..94868f8fcb 100644
     2--- a/src/policy/ephemeral_policy.cpp
     3+++ b/src/policy/ephemeral_policy.cpp
     4@@ -20,11 +20,11 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
     5     return true;
     6 }
     7 
     8-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
     9+bool CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool, TxValidationState& child_state)
    10 {
    11     if (!Assume(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}))) {
    12         // Bail out of spend checks if caller gave us an invalid package
    13-        return std::nullopt;
    14+        return true;
    15     }
    16 
    17     std::map<Txid, CTransactionRef> map_txid_ref;
    18@@ -70,9 +70,11 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
    19         }
    20 
    21         if (!unspent_parent_dust.empty()) {
    22-            return txid;
    23+            child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
    24+                                strprintf("tx %s did not spend parent's ephemeral dust", txid.ToString()));
    25+            return false;
    26         }
    27     }
    28 
    29-    return std::nullopt;
    30+    return true;
    31 }
    32diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
    33index 26140f9a02..bf2c9691a7 100644
    34--- a/src/policy/ephemeral_policy.h
    35+++ b/src/policy/ephemeral_policy.h
    36@@ -48,8 +48,8 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
    37 /** Must be called for each transaction(package) if any dust is in the package.
    38  *  Checks that each transaction's parents have their dust spent by the child,
    39  *  where parents are either in the mempool or in the package itself.
    40- *  The function returns std::nullopt if all dust is properly spent, or the txid of the violating child spend.
    41+ *  The function returns true if all dust is properly spent.
    42  */
    43-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
    44+bool CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool, TxValidationState& child_state);
    45 
    46 #endif // BITCOIN_POLICY_EPHEMERAL_POLICY_H
    47diff --git a/src/validation.cpp b/src/validation.cpp
    48index 2f3e7d61a8..f6d05956ee 100644
    49--- a/src/validation.cpp
    50+++ b/src/validation.cpp
    51@@ -1456,11 +1456,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
    52     }
    53 
    54     if (m_pool.m_opts.require_standard) {
    55-        if (const auto ephemeral_violation{CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
    56-            const Txid& txid = ephemeral_violation.value();
    57-            Assume(txid == ptx->GetHash());
    58-            ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
    59-                          strprintf("tx %s did not spend parent's ephemeral dust", txid.ToString()));
    60+        if (!CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool, ws.m_state)) {
    61             return MempoolAcceptResult::Failure(ws.m_state);
    62         }
    63     }
    64@@ -1605,13 +1601,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
    65 
    66     // Now that we've bounded the resulting possible ancestry count, check package for dust spends
    67     if (m_pool.m_opts.require_standard) {
    68-        if (const auto ephemeral_violation{CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
    69-            const Txid& child_txid = ephemeral_violation.value();
    70-            TxValidationState child_state;
    71-            child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
    72-                          strprintf("tx %s did not spend parent's ephemeral dust", child_txid.ToString()));
    73+        if (TxValidationState child_state; !CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool, child_state)) {
    74             package_state.Invalid(PackageValidationResult::PCKG_TX, "unspent-dust");
    75-            results.emplace(child_txid, MempoolAcceptResult::Failure(child_state));
    76+            results.emplace(txns.back()->GetHash(), MempoolAcceptResult::Failure(child_state));
    77             return PackageMempoolAcceptResult(package_state, std::move(results));
    78         }
    79     }
    

    tdb3 commented at 10:46 pm on November 10, 2024:

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

    Example:

    0 /** Must be called for each transaction once transaction fees are known.
    1  * Does context-less checks about a single transaction.
    2- * Returns false if the fee is non-zero and dust exists, populating state. True otherwise.
    3+ * [@returns](/bitcoin-bitcoin/contributor/returns/) false if the fee is non-zero and dust exists, populating state. True otherwise.
    4  */
    5 bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state);
    

    instagibbs commented at 2:56 pm on November 12, 2024:
    will touch in follow-up

    instagibbs commented at 3:06 pm on November 12, 2024:
    will take a crack at this in follow-up

    instagibbs commented at 4:24 pm on November 13, 2024:
    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! https://github.com/bitcoin/bitcoin/pull/31279
  220. in src/validation.cpp:916 in 131bed19bd outdated
    927@@ -927,6 +928,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
    928                                     fSpendsCoinbase, nSigOpsCost, lock_points.value()));
    929     ws.m_vsize = entry->GetTxSize();
    930 
    931+    // Enforces 0-fee for dust transactions, no incentive to be mined alone
    


    hodlinator commented at 8:15 pm on November 6, 2024:

    More precise:

    0    // Enforces 0-fee for transactions with dust outputs, no incentive to be mined alone
    
  221. in test/functional/mempool_ephemeral_dust.py:31 in 131bed19bd outdated
    26+        self.num_nodes = 2
    27+
    28+        # Don't test trickling logic
    29+        self.noban_tx_relay = True
    30+
    31+    def add_output_to_create_multi_result(self, result, output_value=0):
    


    hodlinator commented at 8:47 pm on November 6, 2024:

    nit: Could add _-prefix to signify an internal method. See MiniWallet._create_utxo, ZMQSubscriber._receive_from_publisher_and_check.

    0    def _add_output_to_create_multi_result(self, result, output_value=0):
    
  222. in test/functional/mempool_ephemeral_dust.py:64 in 131bed19bd outdated
    59+        self.test_multidust()
    60+        self.test_nonzero_dust()
    61+        self.test_non_truc()
    62+        self.test_unspent_ephemeral()
    63+        self.test_reorgs()
    64+        self.test_free_relay()
    


    hodlinator commented at 9:15 pm on November 6, 2024:
    All test_-methods except sponsor_cycle have implementations in the order they are called here, please reorder the call or where in the file the implementation appears.
  223. in test/functional/mempool_ephemeral_dust.py:162 in 131bed19bd outdated
    157+        self.add_output_to_create_multi_result(dusty_tx)
    158+
    159+        sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=dusty_tx["new_utxos"], version=3)
    160+
    161+        self.nodes[0].prioritisetransaction(txid=dusty_tx["txid"], dummy=0, fee_delta=1000)
    162+        self.nodes[1].prioritisetransaction(txid=dusty_tx["txid"], dummy=0, fee_delta=1000)
    


    hodlinator commented at 9:44 pm on November 6, 2024:

    These appear to be no-ops. Verified by adding this just above:

    0assert_equal(self.nodes[0].getrawmempool(), [])
    1assert_equal(self.nodes[1].getrawmempool(), [])
    

    (Could be good if CTxMemPool::PrioritiseTransaction returned false when failing to find a given tx in the pool, so the RPC could throw an exception. Probably in separate PR).


    glozow commented at 12:12 pm on November 12, 2024:
    They aren’t no-ops. You should be checking getprioritisedtransactions instead. You can prio things that aren’t in mempool yet, and mempool tracks them so it can be used in validation later.

    hodlinator commented at 1:27 pm on November 13, 2024:

    Ah, totally missed that. Cheers!

    Not sure what they add, it’s not like there are other transactions in the way. I guess they test that nothing funky is going on with the prioritization-logic resulting in dusty transactions being accepted when they shouldn’t be.

  224. in test/functional/mempool_ephemeral_dust.py:129 in 131bed19bd outdated
    124+        self.restart_node(0)
    125+        self.restart_node(1)
    126+        self.connect_nodes(0, 1)
    127+        assert_mempool_contents(self, self.nodes[0], expected=[])
    128+
    129+    def test_fee_having_parent(self):
    


    hodlinator commented at 9:47 pm on November 6, 2024:

    nit: Less backwards?

    0    def test_parent_with_fee(self):
    
  225. in test/functional/mempool_ephemeral_dust.py:197 in 131bed19bd outdated
    192+    def test_nonzero_dust(self):
    193+        self.log.info("Test that a single output of any satoshi amount is allowed, not checking spending")
    194+
    195+        # We aren't checking spending, allow it in with no fee
    196+        self.restart_node(0, extra_args=["-minrelaytxfee=0"])
    197+        self.restart_node(1, extra_args=["-minrelaytxfee=0"])
    


    hodlinator commented at 9:56 pm on November 6, 2024:
    Could add a comment about why -dustrelayfee is not a concern here?
  226. in test/functional/mempool_ephemeral_dust.py:257 in 131bed19bd outdated
    252+
    253+        # Spend works with dust spent
    254+        sweep_tx_2 = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=dusty_tx["new_utxos"], version=3)
    255+        assert sweep_tx["hex"] != sweep_tx_2["hex"]
    256+        res = self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx_2["hex"]])
    257+        assert_equal(res["package_msg"], "success")
    


    hodlinator commented at 10:15 pm on November 6, 2024:

    Slightly easier to follow comments?

     0        # Setup valid sweep in mempool
     1        sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=dusty_tx["new_utxos"], version=3)
     2        self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]])
     3        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
     4
     5        # RBF-attempt fails when not spending in-mempool dust output from parent
     6        unspent_sweep_tx = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=[dusty_tx["new_utxos"][0]], version=3)
     7        assert_greater_than(unspent_sweep_tx["fee"], sweep_tx["fee"])
     8        res = self.nodes[0].submitpackage([dusty_tx["hex"], unspent_sweep_tx["hex"]])
     9        assert_equal(res["tx-results"][unspent_sweep_tx["wtxid"]]["error"], f"missing-ephemeral-spends, tx {unspent_sweep_tx['txid']} did not spend parent's ephemeral dust")
    10        assert_raises_rpc_error(-26, f"missing-ephemeral-spends, tx {unspent_sweep_tx['txid']} did not spend parent's ephemeral dust", self.nodes[0].sendrawtransaction, unspent_sweep_tx["hex"])
    11        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
    12
    13        # RBF works when dust is spent
    14        sweep_tx_2 = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=dusty_tx["new_utxos"], version=3)
    15        assert sweep_tx["hex"] != sweep_tx_2["hex"]
    16        res = self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx_2["hex"]])
    17        assert_equal(res["package_msg"], "success")
    
  227. in test/functional/mempool_ephemeral_dust.py:293 in 131bed19bd outdated
    288+        assert_equal(self.nodes[0].getrawmempool(), [])
    289+
    290+        dusty_tx = self.wallet.create_self_transfer_multi(
    291+            fee_per_output=0,
    292+            version=3
    293+        )
    


    hodlinator commented at 10:19 pm on November 6, 2024:

    Why the sudden change in style?

    0        dusty_tx = self.wallet.create_self_transfer_multi(fee_per_output=0, version=3)
    

    instagibbs commented at 3:23 pm on November 12, 2024:
    lots of code history blown away I’m sure :)
  228. in test/functional/mempool_ephemeral_dust.py:353 in 131bed19bd outdated
    348+        dusty_tx = self.wallet.create_self_transfer_multi(fee_per_output=0, version=3)
    349+        self.add_output_to_create_multi_result(dusty_tx)
    350+        assert_raises_rpc_error(-26, "min relay fee not met", self.nodes[0].sendrawtransaction, dusty_tx["hex"])
    351+
    352+        block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [dusty_tx["hex"]])
    353+        self.nodes[0].invalidateblock(block_res["hash"])
    


    hodlinator commented at 11:04 pm on November 6, 2024:

    nit: Could validate/document that tx made it into the block to be sure, before invalidating.

    0        block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [dusty_tx["hex"]])
    1        assert_equal(self.nodes[0].getrawtransaction(dusty_tx["txid"], blockhash=block_res["hash"]), dusty_tx["hex"])
    2        self.nodes[0].invalidateblock(block_res["hash"])
    

    instagibbs commented at 3:22 pm on November 12, 2024:
    will look in follow-up

    instagibbs commented at 6:17 pm on November 12, 2024:
    mmm, seems like a test of generateblock to me? going to leave as is
  229. hodlinator commented at 11:27 pm on November 6, 2024: contributor

    Code reviewed up until most of aa7c90a91adcf8ad9d6b94d5d17797cfd0eb5228.

    Disclaimer: I posses far from full context when it comes to validation code, but at least as far as I can tell we are only modifying policy, not consensus.

    PR title should probably have “p2p:” or some other prefix.

  230. in test/functional/mempool_ephemeral_dust.py:366 in 131bed19bd outdated
    361+        # Mine the sweep then re-org, the sweep will not make it back in due to spend checks
    362+        block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [dusty_tx["hex"], sweep_tx["hex"]])
    363+        self.nodes[0].invalidateblock(block_res["hash"])
    364+        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"]], sync=False)
    365+
    366+        # Also should happen if dust is swept
    


    hodlinator commented at 9:49 am on November 7, 2024:
    “Also”? But in this case the sweep-transaction survives the reorg, unlike the case above.

    instagibbs commented at 3:08 pm on November 12, 2024:
    Yeah old language, will touch in follow-up
  231. in test/functional/mempool_ephemeral_dust.py:469 in 131bed19bd outdated
    464+
    465+        res = self.nodes[0].submitpackage([dusty_tx["hex"] for dusty_tx in dusty_txs] + [sweep_tx["hex"]])
    466+        assert_equal(res['package_msg'], "success")
    467+        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"] for dusty_tx in dusty_txs] + [sweep_tx["tx"], cancel_sweep["tx"]])
    468+
    469+        self.generate(self.nodes[0], 25)
    


    hodlinator commented at 10:04 am on November 7, 2024:
    Why 25 and not 1 block?

    instagibbs commented at 3:08 pm on November 12, 2024:
    no reason, leaving as is

    hodlinator commented at 2:39 pm on November 19, 2024:
    Seems unnecessarily wasteful for all future CPU cycles consumed by this test.

    instagibbs commented at 2:55 pm on November 19, 2024:
    pushed on follow-up

    hodlinator commented at 2:58 pm on November 19, 2024:
    Cheers!
  232. in src/test/fuzz/package_eval.cpp:213 in 131bed19bd outdated
    211+    auto tx_pool_{MakeEphemeralMempool(node)};
    212+    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(tx_pool_.get());
    213+
    214+    chainstate.SetMempool(&tx_pool);
    215+
    216+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)
    


    hodlinator commented at 10:40 am on November 7, 2024:

    (Just curious - this is a very common pattern, why do we leave it up to the fuzz engine instead of just doing:

    0    LIMITED_WHILE(true, 300)
    

    Does it help define discrete sections of the fuzz data, which ends up being useful somehow?

    These kind of uses make more sense to me: LIMITED_WHILE(provider.remaining_bytes() > 0, iter_limit), LIMITED_WHILE(!available_coins.empty(), 500).)


    instagibbs commented at 3:09 pm on November 12, 2024:
    good question, I’ll take a look on follow-up

    instagibbs commented at 4:27 pm on November 13, 2024:

    11:24 <@dergoegge> I’ve suggested LIMITED_WHILE(provider.remaining_bytes() > 0, iter_limit) in the past as well 11:24 <@dergoegge> I think the goal is to let the fuzzer “choose” the number of iterations and both the above and consumebool achieve that 11:24 <@dergoegge> Iirc there were concerns around using remaining_bytes as really large inputs might cause timeouts 11:24 <@dergoegge> which is something we’ve seen with consumebool as well if the iteration limit is too high

    edit: actually, I’m going to take the suggested change, more loops can only be better

  233. in src/test/fuzz/package_eval.cpp:221 in 131bed19bd outdated
    219+
    220+        std::vector<CTransactionRef> txs;
    221+
    222+        // Find something we may want to double-spend with two input single tx
    223+        std::optional<COutPoint> outpoint_to_rbf{GetChildEvictingPrevout(tx_pool)};
    224+        bool should_rbf_eph_spend = outpoint_to_rbf && fuzzed_data_provider.ConsumeBool();
    


    hodlinator commented at 11:07 am on November 7, 2024:

    nit: Reduce the number of variables:

    0        std::optional<COutPoint> outpoint_to_rbf{fuzzed_data_provider.ConsumeBool() ? GetChildEvictingPrevout(tx_pool) : std::nullopt};
    

    instagibbs commented at 3:18 pm on November 12, 2024:
    will look in follow-up
  234. in src/policy/policy.cpp:146 in d5c564a24f outdated
    142@@ -142,11 +143,16 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
    143             reason = "bare-multisig";
    144             return false;
    145         } else if (IsDust(txout, dust_relay_fee)) {
    146-            reason = "dust";
    147-            return false;
    148+            num_dust_outputs++;
    


    stickies-v commented at 12:06 pm on November 7, 2024:

    In d5c564a24f96ec384b2de68bf87e6eb3ad9239d2

    developer-notes-nit: ++i is preferred over i++.


    instagibbs commented at 3:06 pm on November 12, 2024:
    leaving as-is
  235. in src/test/fuzz/package_eval.cpp:237 in 131bed19bd outdated
    235+            bool last_tx = num_txs > 1 && txs.size() == num_txs - 1;
    236+
    237+            // Create transaction to add to the mempool
    238+            const CTransactionRef tx = [&] {
    239+                CMutableTransaction tx_mut;
    240+                tx_mut.version = CTransaction::CURRENT_VERSION;
    


    hodlinator commented at 1:42 pm on November 7, 2024:
    Only exercising v2 - maybe you could describe the reasoning behind the test at the top of fuzz target?

    instagibbs commented at 3:17 pm on November 12, 2024:
    the version doesn’t really matter since we set mempool_opts.min_relay_feerate = CFeeRate(0);
  236. in src/test/fuzz/package_eval.cpp:230 in 131bed19bd outdated
    228+
    229+        std::set<COutPoint> package_outpoints;
    230+        while (txs.size() < num_txs) {
    231+
    232+            // Last transaction in a package needs to be a child of parents to get further in validation
    233+            // so the last transaction to be generated(in a >1 package) must spend all package-made outputs
    


    hodlinator commented at 1:52 pm on November 7, 2024:
    0            // so the last transaction to be generated (in a >1 package) must spend all package-made outputs
    
  237. in src/test/txvalidation_tests.cpp:96 in 131bed19bd outdated
    89@@ -89,6 +90,125 @@ static inline CTransactionRef make_tx(const std::vector<COutPoint>& inputs, int3
    90     return MakeTransactionRef(mtx);
    91 }
    92 
    93+// Same as make_tx but adds 2 normal outputs and 0-value dust to end of vout
    94+static inline CTransactionRef make_ephemeral_tx(const std::vector<COutPoint>& inputs, int32_t version)
    95+{
    96+    CMutableTransaction mtx = CMutableTransaction{};
    


    hodlinator commented at 2:04 pm on November 7, 2024:
    0    CMutableTransaction mtx;
    

    Found via DrahtBot’s codecoverage link & SonarCloud.

  238. in src/test/fuzz/package_eval.cpp:323 in 131bed19bd outdated
    321+        }
    322+
    323+        // Remember all added transactions
    324+        std::set<CTransactionRef> added;
    325+        auto txr = std::make_shared<TransactionsDelta>(added);
    326+        node.validation_signals->RegisterSharedValidationInterface(txr);
    


    hodlinator commented at 2:16 pm on November 7, 2024:
    This seems to be some cargo cult code from other tests, added and txr appear fine to remove.

    instagibbs commented at 3:10 pm on November 12, 2024:
    used to have a use, let me look at it in follow-up
  239. in src/test/transaction_tests.cpp:819 in d5c564a24f outdated
    812@@ -813,6 +813,11 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
    813     // Check dust with default relay fee:
    814     CAmount nDustThreshold = 182 * g_dust.GetFeePerK() / 1000;
    815     BOOST_CHECK_EQUAL(nDustThreshold, 546);
    816+
    817+    // Add dust output to take dust slot, still standard!
    818+    t.vout.emplace_back(0, t.vout[0].scriptPubKey);
    819+    CheckIsStandard(t);
    


    stickies-v commented at 2:32 pm on November 7, 2024:

    In d5c564a24f96ec384b2de68bf87e6eb3ad9239d2

    Since MAX_DUST_OUTPUTS_PER_TX is parameterized, it would make the tests more robust to dynamically add dust outputs too?

     0diff --git a/src/policy/policy.h b/src/policy/policy.h
     1index 0488f8dbee..e023c99560 100644
     2--- a/src/policy/policy.h
     3+++ b/src/policy/policy.h
     4@@ -80,7 +80,7 @@ static constexpr unsigned int EXTRA_DESCENDANT_TX_SIZE_LIMIT{10000};
     5 /**
     6  * Maximum number of ephemeral dust outputs allowed.
     7  */
     8-static constexpr unsigned int MAX_DUST_OUTPUTS_PER_TX{1};
     9+static constexpr unsigned int MAX_DUST_OUTPUTS_PER_TX{4};
    10 
    11 /**
    12  * Mandatory script verification flags that all new transactions must comply with for
    13diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
    14index 3e4c085c0e..f38c015f6d 100644
    15--- a/src/test/transaction_tests.cpp
    16+++ b/src/test/transaction_tests.cpp
    17@@ -814,9 +814,11 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
    18     CAmount nDustThreshold = 182 * g_dust.GetFeePerK() / 1000;
    19     BOOST_CHECK_EQUAL(nDustThreshold, 546);
    20 
    21-    // Add dust output to take dust slot, still standard!
    22-    t.vout.emplace_back(0, t.vout[0].scriptPubKey);
    23-    CheckIsStandard(t);
    24+    // Add dust outputs up to allowed maximum, still standard!
    25+    for (size_t i{0}; i < MAX_DUST_OUTPUTS_PER_TX; ++i) {
    26+        t.vout.emplace_back(0, t.vout[0].scriptPubKey);
    27+        CheckIsStandard(t);
    28+    }
    29 
    30     // dust:
    31     t.vout[0].nValue = nDustThreshold - 1;
    32@@ -974,9 +976,9 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
    33     CheckIsNotStandard(t, "bare-multisig");
    34     g_bare_multi = DEFAULT_PERMIT_BAREMULTISIG;
    35 
    36-    // Add dust output to take dust slot
    37+    // Add dust outputs up to allowed maximum
    38     assert(t.vout.size() == 1);
    39-    t.vout.emplace_back(0, t.vout[0].scriptPubKey);
    40+    t.vout.insert(t.vout.end(), MAX_DUST_OUTPUTS_PER_TX, {0, t.vout[0].scriptPubKey});
    41 
    42     // Check compressed P2PK outputs dust threshold (must have leading 02 or 03)
    43     t.vout[0].scriptPubKey = CScript() << std::vector<unsigned char>(33, 0x02) << OP_CHECKSIG;
    

    instagibbs commented at 3:22 pm on November 12, 2024:
    will look in followup
  240. in src/test/fuzz/package_eval.cpp:243 in 131bed19bd outdated
    241+                tx_mut.nLockTime = 0;
    242+                // Last tx will sweep half or more of all outpoints from package
    243+                const auto num_in = should_rbf_eph_spend ? 2 :
    244+                    last_tx ? fuzzed_data_provider.ConsumeIntegralInRange<int>(package_outpoints.size()/2 + 1, package_outpoints.size()) :
    245+                    fuzzed_data_provider.ConsumeIntegralInRange<int>(1, 4);
    246+                auto num_out = should_rbf_eph_spend ? 1 : fuzzed_data_provider.ConsumeIntegralInRange<int>(1, 4);
    


    hodlinator commented at 2:35 pm on November 7, 2024:
    nit: Declare both const to be consistent between num_in & num_out

    instagibbs commented at 3:11 pm on November 12, 2024:
    will take a look in follow-up
  241. in src/test/fuzz/package_eval.cpp:232 in 131bed19bd outdated
    230+        while (txs.size() < num_txs) {
    231+
    232+            // Last transaction in a package needs to be a child of parents to get further in validation
    233+            // so the last transaction to be generated(in a >1 package) must spend all package-made outputs
    234+            // Note that this test currently only spends package outputs in last transaction.
    235+            bool last_tx = num_txs > 1 && txs.size() == num_txs - 1;
    


    hodlinator commented at 4:24 pm on November 7, 2024:
    Might as well narrow the lifetime of last_tx by moving it into the lambda below.

    instagibbs commented at 3:20 pm on November 12, 2024:
    will take a look in followup
  242. in src/test/fuzz/package_eval.cpp:235 in 131bed19bd outdated
    233+            // so the last transaction to be generated(in a >1 package) must spend all package-made outputs
    234+            // Note that this test currently only spends package outputs in last transaction.
    235+            bool last_tx = num_txs > 1 && txs.size() == num_txs - 1;
    236+
    237+            // Create transaction to add to the mempool
    238+            const CTransactionRef tx = [&] {
    


    hodlinator commented at 4:27 pm on November 7, 2024:

    Instead of creating two variables named tx here and on line 288, emplace_back the return value of the lambda:

    0            txs.push_back([&] {
    

    instagibbs commented at 3:04 pm on November 12, 2024:
    will add in follow-up
  243. in src/test/fuzz/package_eval.cpp:251 in 131bed19bd outdated
    249+
    250+                Assert((int)outpoints.size() >= num_in && num_in > 0);
    251+
    252+                CAmount amount_in{0};
    253+                for (int i = 0; i < num_in; ++i) {
    254+                    // Pop random outpoint
    


    hodlinator commented at 2:32 pm on November 8, 2024:
    0                    // Pop random outpoint. We erase them to avoid double-spending
    1                    // while in this loop, but later add them back (unless last_tx).
    

    instagibbs commented at 3:04 pm on November 12, 2024:
    will add in followup
  244. in src/test/fuzz/package_eval.cpp:311 in 131bed19bd outdated
    309+            const auto& txid = fuzzed_data_provider.ConsumeBool() ?
    310+                                   txs.back()->GetHash() :
    311+                                   PickValue(fuzzed_data_provider, mempool_outpoints).hash;
    312+            const auto delta = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(-50 * COIN, +50 * COIN);
    313+            // We only prioritise out of mempool transactions since PrioritiseTransaction doesn't
    314+            // filter for ephemeral dust GetEntry
    


    hodlinator commented at 2:35 pm on November 8, 2024:

    “GetEntry” what?

    0            // filter for ephemeral dust
    

    instagibbs commented at 3:03 pm on November 12, 2024:
    will remove in follow up
  245. in src/policy/ephemeral_policy.cpp:13 in 131bed19bd outdated
     8+bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
     9+{
    10+    return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
    11+}
    12+
    13+bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
    


    stickies-v commented at 2:52 pm on November 8, 2024:

    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.

     0diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
     1index 6854822e35..c9f1ba076d 100644
     2--- a/src/policy/ephemeral_policy.cpp
     3+++ b/src/policy/ephemeral_policy.cpp
     4@@ -5,12 +5,12 @@
     5 #include <policy/ephemeral_policy.h>
     6 #include <policy/policy.h>
     7 
     8-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
     9+bool HasDust(const CTransaction& tx, CFeeRate dust_relay_rate)
    10 {
    11-    return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
    12+    return std::any_of(tx.vout.cbegin(), tx.vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
    13 }
    14 
    15-bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
    16+bool CheckValidEphemeralTx(const CTransaction& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
    17 {
    18     // We never want to give incentives to mine this transaction alone
    19     if ((base_fee != 0 || mod_fee != 0) && HasDust(tx, dust_relay_rate)) {
    20diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
    21index 26140f9a02..10a5edf337 100644
    22--- a/src/policy/ephemeral_policy.h
    23+++ b/src/policy/ephemeral_policy.h
    24@@ -35,7 +35,7 @@
    25  */
    26 
    27 /** Returns true if transaction contains dust */
    28-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate);
    29+bool HasDust(const CTransaction& tx, CFeeRate dust_relay_rate);
    30 
    31 /* All the following checks are only called if standardness rules are being applied. */
    32 
    33@@ -43,7 +43,7 @@ bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate);
    34  * Does context-less checks about a single transaction.
    35  * Returns false if the fee is non-zero and dust exists, populating state. True otherwise.
    36  */
    37-bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state);
    38+bool CheckValidEphemeralTx(const CTransaction& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state);
    39 
    40 /** Must be called for each transaction(package) if any dust is in the package.
    41  *  Checks that each transaction's parents have their dust spent by the child,
    42diff --git a/src/validation.cpp b/src/validation.cpp
    43index 2f3e7d61a8..8387a7f971 100644
    44--- a/src/validation.cpp
    45+++ b/src/validation.cpp
    46@@ -930,7 +930,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
    47 
    48     // Enforces 0-fee for dust transactions, no incentive to be mined alone
    49     if (m_pool.m_opts.require_standard) {
    50-        if (!CheckValidEphemeralTx(ptx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, ws.m_modified_fees, state)) {
    51+        if (!CheckValidEphemeralTx(tx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, ws.m_modified_fees, state)) {
    52             return false; // state filled in by CheckValidEphemeralTx
    53         }
    54     }
    

    instagibbs commented at 2:50 pm on November 12, 2024:
    will touch on follow-up
  246. in src/validation.cpp:919 in d5c564a24f outdated
    927@@ -927,6 +928,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
    928                                     fSpendsCoinbase, nSigOpsCost, lock_points.value()));
    929     ws.m_vsize = entry->GetTxSize();
    930 
    931+    // Enforces 0-fee for dust transactions, no incentive to be mined alone
    932+    if (m_pool.m_opts.require_standard) {
    933+        if (!CheckValidEphemeralTx(ptx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, ws.m_modified_fees, state)) {
    934+            return false; // state filled in by CheckValidEphemeralTx
    


    stickies-v commented at 3:08 pm on November 8, 2024:

    I’d prefer actually asserting this instead of just documenting it (unsure if we prefer !IsValid() or IsInvalid() here, the latter not capturing IsError()).

     0diff --git a/src/validation.cpp b/src/validation.cpp
     1index 2f3e7d61a8..ac619fa385 100644
     2--- a/src/validation.cpp
     3+++ b/src/validation.cpp
     4@@ -931,7 +931,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     5     // Enforces 0-fee for dust transactions, no incentive to be mined alone
     6     if (m_pool.m_opts.require_standard) {
     7         if (!CheckValidEphemeralTx(ptx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, ws.m_modified_fees, state)) {
     8-            return false; // state filled in by CheckValidEphemeralTx
     9+            Assert(!state.IsValid());
    10+            return false;
    11         }
    12     }
    13 
    
  247. in src/test/util/txmempool.h:60 in 131bed19bd outdated
    55+void CheckMempoolEphemeralInvariants(const CTxMemPool& tx_pool);
    56+
    57+/** Return indexes of the transaction's outputs that are considered dust
    58+ * at given dust_relay_rate.
    59+*/
    60+std::vector<uint32_t> GetDustIndexes(const CTransactionRef& tx_ref, CFeeRate dust_relay_rate);
    


    hodlinator commented at 3:21 pm on November 8, 2024:
    Could introduce & when adding the function instead of adding them in later commit (d73650d4ce1358f4acf466276954c20bfe110ba3)?

    instagibbs commented at 3:03 pm on November 12, 2024:
    was a history error, not touching
  248. in src/test/txvalidation_tests.cpp:105 in 131bed19bd outdated
    100+    for (size_t i{0}; i < inputs.size(); ++i) {
    101+        mtx.vin[i].prevout = inputs[i];
    102+    }
    103+    for (auto i{0}; i < 3; ++i) {
    104+        mtx.vout[i].scriptPubKey = CScript() << OP_TRUE;
    105+        mtx.vout[i].nValue = (i == 2) ? 0 : 10000;
    


    hodlinator commented at 3:46 pm on November 8, 2024:
    Pass in dust_index to make this 2 less magical, or make it a file-local constant used both in this utility function and in the new test case?

    instagibbs commented at 3:02 pm on November 12, 2024:
    will do in follow-up
  249. in src/test/txvalidation_tests.cpp:131 in 131bed19bd outdated
    126+    auto dust_spend = make_tx({COutPoint{dust_txid, dust_index}}, /*version=*/2);
    127+
    128+    // We first start with nothing "in the mempool", using package checks
    129+
    130+    // Trivial single transaction with no dust
    131+    BOOST_CHECK(!CheckEphemeralSpends({dust_spend}, minrelay, pool).has_value());
    


    hodlinator commented at 3:51 pm on November 8, 2024:

    Could rely on optional’s bool operator to decrease noise here and below:

    0    BOOST_CHECK(!CheckEphemeralSpends({dust_spend}, minrelay, pool));
    

    instagibbs commented at 3:00 pm on November 12, 2024:
    will touch in follow-up
  250. in src/test/txvalidation_tests.cpp:143 in 131bed19bd outdated
    138+    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, dust_spend}, minrelay, pool).has_value());
    139+
    140+    auto dust_non_spend = make_tx({COutPoint{dust_txid, dust_index - 1}}, /*version=*/2);
    141+
    142+    // Child spending non-dust only from parent should be disallowed even if dust otherwise spent
    143+    BOOST_CHECK(CheckEphemeralSpends({grandparent_tx_1, dust_non_spend, dust_spend}, minrelay, pool).has_value());
    


    hodlinator commented at 3:55 pm on November 8, 2024:

    Might as well be explicit about expected txid here and elsewhere?

    0    BOOST_CHECK_EQUAL(CheckEphemeralSpends({grandparent_tx_1, dust_non_spend, dust_spend}, minrelay, pool), dust_non_spend->GetHash());
    

    instagibbs commented at 2:59 pm on November 12, 2024:
    will touch in follow-up
  251. in src/bench/mempool_ephemeral_spends.cpp:46 in 131bed19bd outdated
    41+    if (bench.complexityN() > 1) {
    42+        number_outputs = static_cast<int>(bench.complexityN());
    43+    }
    44+
    45+    // Tx with many outputs
    46+    CMutableTransaction tx1 = CMutableTransaction();
    


    hodlinator commented at 4:05 pm on November 8, 2024:

    Here and for tx2:

    0    CMutableTransaction tx1;
    

    instagibbs commented at 2:59 pm on November 12, 2024:
    maybe touch in follow-up
  252. in src/bench/mempool_ephemeral_spends.cpp:78 in 131bed19bd outdated
    73+
    74+    uint32_t iteration{0};
    75+
    76+    bench.run([&]() NO_THREAD_SAFETY_ANALYSIS {
    77+
    78+        CheckEphemeralSpends({tx2_r}, /*dust_relay_rate=*/CFeeRate(iteration * COIN / 10), pool);
    


    hodlinator commented at 4:30 pm on November 8, 2024:

    With the diff:

     0diff --git a/src/bench/mempool_ephemeral_spends.cpp b/src/bench/mempool_ephemeral_spends.cpp
     1index e867c61752..dc5e577caf 100644
     2--- a/src/bench/mempool_ephemeral_spends.cpp
     3+++ b/src/bench/mempool_ephemeral_spends.cpp
     4@@ -72,12 +72,16 @@ static void MempoolCheckEphemeralSpends(benchmark::Bench& bench)
     5     AddTx(tx1_r, pool);
     6 
     7     uint32_t iteration{0};
     8+    uint32_t failure{0};
     9 
    10     bench.run([&]() NO_THREAD_SAFETY_ANALYSIS {
    11 
    12-        CheckEphemeralSpends({tx2_r}, /*dust_relay_rate=*/CFeeRate(iteration * COIN / 10), pool);
    13+        if (CheckEphemeralSpends({tx2_r}, /*dust_relay_rate=*/CFeeRate(iteration * COIN / 10), pool))
    14+            ++failure;
    15         iteration++;
    16     });
    17+
    18+    printf("success: %d, failure: %d\n", iteration - failure, failure);
    19 }
    20 
    21 BENCHMARK(MempoolCheckEphemeralSpends, benchmark::PriorityLevel::HIGH);
    

    The result was:

    0 build/src/bench/bench_bitcoin -filter=MempoolCheckEphemeralSpends -min-time=30000
    1|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
    2|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
    3|          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`
    4success: 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?


    glozow commented at 1:00 pm on November 12, 2024:
    Did you consider benching with the parent in package? I suppose it’s about the same.

    instagibbs commented at 2:36 pm on November 12, 2024:
    should be the same, or even better since it’s not being fetched from the mempool?

    instagibbs commented at 2:53 pm on November 12, 2024:
    It shouldn’t effect the complexity much, as the cost is scanning all the parents’ outputs
  253. stickies-v commented at 6:36 pm on November 8, 2024: contributor
    Concept ACK. I’ve mostly reviewed the non-test code so far. No comments are blocking, I appreciate on a PR of this scope we can’t address everything and things may need to happen in a follow-up.
  254. hodlinator commented at 10:12 pm on November 8, 2024: contributor

    Concept ACK 131bed19bdfc922328cad9781fa9122b6810a8fd

    Thanks for working on this! Seems like an obvious win for L2s.

    Haven’t yet absorbed the full context sufficiently to fully A-C-K it myself. Hopefully my comments here and above provide value even though most are surface level. My personal priority with this review is to get it in better shape for others so they can A-C-K more easily if they agree.

  255. in test/functional/mempool_dust.py:89 in 28733511de outdated
    84+        dust_txid = self.nodes[0].sendrawtransaction(hexstring=dusty_tx["hex"], maxfeerate=0)
    85+
    86+        assert_equal(self.nodes[0].getrawmempool(), [dust_txid])
    87+
    88+        # Spends one dust along with fee input, leave other dust unspent to check ephemeral dust checks aren't being enforced
    89+        sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[self.wallet.get_utxo(), dusty_tx["new_utxos"][0]])
    


    tdb3 commented at 10:19 pm on November 10, 2024:
    Thought about what guarantees we would have for get_utxo() to not return one of the dust. At first glance, looks like currently get_utxo() would return the largest utxo (i.e. not dust). Could have a stronger guarantee if we used get_utxo(confirmed_only=True) instead (dusty_tx is in the mempool but not yet confirmed). Could be left for a follow-up unless updating for another reason.

    instagibbs commented at 2:58 pm on November 12, 2024:
    good point, will take a look in followup
  256. in src/policy/ephemeral_policy.h:51 in d5c564a24f outdated
    46+bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state);
    47+
    48+/** Must be called for each transaction(package) if any dust is in the package.
    49+ *  Checks that each transaction's parents have their dust spent by the child,
    50+ *  where parents are either in the mempool or in the package itself.
    51+ *  The function returns std::nullopt if all dust is properly spent, or the txid of the violating child spend.
    


    tdb3 commented at 11:12 pm on November 10, 2024:
    The function also returns nullopt for an invalid package. Maybe I’m missing something, but this seems to conjoin a failure case (invalid package) with the success case (all dust properly spent). Something like #30239 (review) could increase consistency.

    instagibbs commented at 2:58 pm on November 12, 2024:
    I’ll consider the other comment’s suggestions, but this really is just a belt and suspenders, the caller is given no guarantees on if you’re calling with nullptrs.
  257. in src/policy/ephemeral_policy.cpp:16 in d5c564a24f outdated
    11+}
    12+
    13+bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
    14+{
    15+    // We never want to give incentives to mine this transaction alone
    16+    if ((base_fee != 0 || mod_fee != 0) && HasDust(tx, dust_relay_rate)) {
    


    tdb3 commented at 11:20 pm on November 10, 2024:
    If it’s decided to allow non-zero modified fee, then the latter half of the || might need to be adjusted here (https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831104732).

    instagibbs commented at 2:50 pm on November 12, 2024:
    think we’re sticking with 0-fee both base and modified
  258. in src/policy/ephemeral_policy.cpp:63 in d5c564a24f outdated
    58+                        unspent_parent_dust.insert(COutPoint(parent_txid, out_index));
    59+                    }
    60+                }
    61+            }
    62+
    63+            processed_parent_set.insert(parent_txid);
    


    tdb3 commented at 11:38 pm on November 10, 2024:

    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.


    instagibbs commented at 2:45 pm on November 12, 2024:
    If it’s not in mempool or the package, then it can’t have unconfirmed dust. Is there a scenario you’re concerned about?

    tdb3 commented at 1:15 am on November 13, 2024:
    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).
  259. tdb3 commented at 0:09 am on November 11, 2024: contributor

    Concept ACK

    Great policy update that will provide flexibility to L2s. Left a few comments, and want to take a deeper look at test cases.

  260. DrahtBot added the label CI failed on Nov 11, 2024
  261. DrahtBot commented at 4:47 pm on November 11, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32482002322

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  262. glozow commented at 1:02 pm on November 12, 2024: member

    ACK 131bed19bdfc922328cad9781fa9122b6810a8fd

    ~Please lmk what you want to do with the remaining comments, I’m happy to reack or merge with a followup lined up.~ Happy to reack after rebase.

  263. DrahtBot requested review from hodlinator on Nov 12, 2024
  264. DrahtBot requested review from tdb3 on Nov 12, 2024
  265. DrahtBot requested review from stickies-v on Nov 12, 2024
  266. functional test: Add new -dustrelayfee=0 test case
    This test would catch regressions where ephemeral
    dust checks are being erroneously applied on outputs
    that are not actually dust.
    04b2714fbb
  267. policy: Allow dust in transactions, spent in-mempool
    Also known as Ephemeral Dust.
    
    We try to ensure that dust is spent in blocks by requiring:
      - ephemeral dust tx is 0-fee
      - ephemeral dust tx only has one dust output
      - If the ephemeral dust transaction has a child,
        the dust is spent by by that child.
    
    0-fee requirement means there is no incentive to mine
    a transaction which doesn't have a child bringing its
    own fees for the transaction package.
    e1d3e81ab4
  268. rpc: disallow in-mempool prioritisation of dusty tx 4e68f90139
  269. functional test: Add ephemeral dust tests e2e30e89ba
  270. test: Add CheckMempoolEphemeralInvariants
    Checks that transactions in mempool with dust
    follow expected invariants.
    127719f516
  271. in src/test/fuzz/package_eval.cpp:154 in 131bed19bd outdated
    149+
    150+    // And set minrelay to 0 to allow ephemeral parent tx even with non-TRUC
    151+    mempool_opts.min_relay_feerate = CFeeRate(0);
    152+
    153+    // Don't worry about signaling replacement
    154+    mempool_opts.full_rbf = true;
    


    glozow commented at 1:09 pm on November 12, 2024:
    Ah RIP, this is a merge conflict with #30592. That’s the CI error.
  272. instagibbs force-pushed on Nov 12, 2024
  273. instagibbs commented at 2:26 pm on November 12, 2024: member

    image

    rebased due to conflict with merge of #30592

    going to go over all outstanding comments

  274. fuzz: add ephemeral_package_eval harness
    Works a bit harder to get ephemeral dust
    transactions into the mempool.
    21d28b2f36
  275. test: unit test for CheckEphemeralSpends 71a6ab4b33
  276. Add release note for ephemeral dust 3f6559fa58
  277. bench: Add basic CheckEphemeralSpends benchmark 5c2e291060
  278. instagibbs force-pushed on Nov 12, 2024
  279. instagibbs commented at 3:20 pm on November 12, 2024: member
    compiling a list of things to do in follow-up, otherwise keeping as-is
  280. instagibbs commented at 3:49 pm on November 12, 2024: member
    addrman test failure for win64, assuming unrelated: https://github.com/bitcoin/bitcoin/pull/30239/checks?check_run_id=32868463159
  281. fanquake commented at 4:11 pm on November 12, 2024: member

    addrman test failure for win64, assuming unrelated:

    Yea this is a Wine issue (see #31071 & the linked issues). I restarted the job.

  282. DrahtBot removed the label CI failed on Nov 12, 2024
  283. glozow commented at 5:43 pm on November 12, 2024: member
    reACK 5c2e291060c via range-diff. Nothing but a rebase and removing the conflict.
  284. DrahtBot requested review from theStack on Nov 12, 2024
  285. DrahtBot requested review from sdaftuar on Nov 12, 2024
  286. theStack approved
  287. theStack commented at 6:13 pm on November 12, 2024: contributor
    re-ACK 5c2e291060cca3be500f3af0f6f2d3fd2177a7c9
  288. glozow merged this on Nov 13, 2024
  289. glozow closed this on Nov 13, 2024

  290. TheCharlatan referenced this in commit a73b2bd0f0 on Nov 14, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me