Ephemeral Dust #30239
Ephemeral Dust #30239
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30239. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
instagibbs
force-pushed
the
2024-03-general-ephemeral
branch
from
f9119c7
to
338311b
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. Debug: https://github.com/bitcoin/bitcoin/runs/25906194302 |
|
going to put this into draft until #29496 is merged, since dust checks are completely off when |
instagibbs
force-pushed
the
2024-03-general-ephemeral
branch
2 times, most recently
from
7c22f4a
to
b324c07
Compare
|
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.) |
|
@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. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. Debug: https://github.com/bitcoin/bitcoin/runs/25953908933 |
instagibbs
force-pushed
the
2024-03-general-ephemeral
branch
from
b324c07
to
9212708
Compare
|
rebased due to s/nVersion/version/ change for transactions causing a silent merge conflict |
instagibbs
force-pushed
the
2024-03-general-ephemeral
branch
from
9212708
to
02600cd
Compare
|
rebased on master due to conflict |
There was a problem hiding this comment.
Concept ACK
Mostly looked and reasoned about the core commit 92d7ad2 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.
| @@ -21,6 +21,20 @@ | |||
|
|
|||
| ORPHAN_TX_EXPIRE_TIME = 1200 | |||
|
|
|||
| def assert_mempool_contents(test_framework, node, expected=None, sync=True): | |||
There was a problem hiding this comment.
follow-up idea: could take use of that helper in mempool_package_rbf.py (might be a nice "good first issue")
There was a problem hiding this comment.
Deferring to Future Work
There was a problem hiding this comment.
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.
| dusty_tx = self.wallet.create_self_transfer_multi(fee_per_output=0, version=3) | ||
| self.add_output_to_create_multi_result(dusty_tx) | ||
|
|
||
| sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=dusty_tx["new_utxos"], version=3) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
deferring to Future Work
This test would catch regressions where ephemeral dust checks are being erroneously applied on outputs that are not actually dust.
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.
instagibbs
force-pushed
the
2024-03-general-ephemeral
branch
from
40377e8
to
51c2394
Compare
|
ACK 51c2394 |
Checks that transactions in mempool with dust follow expected invariants.
Works a bit harder to get ephemeral dust transactions into the mempool.
instagibbs
force-pushed
the
2024-03-general-ephemeral
branch
from
51c2394
to
131bed1
Compare
| * 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: |
There was a problem hiding this comment.
nit: missing closing double quote
There was a problem hiding this comment.
will fixup if I end up touching things
| @@ -141,7 +141,7 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns, | |||
| return std::nullopt; | |||
| } | |||
|
|
|||
| std::vector<uint32_t> GetDustIndexes(const CTransactionRef tx_ref, CFeeRate dust_relay_rate) | |||
| std::vector<uint32_t> GetDustIndexes(const CTransactionRef& tx_ref, CFeeRate dust_relay_rate) | |||
There was a problem hiding this comment.
nit: should ideally be already part of the commit that introduces this function for minimal diff (here and in header)
There was a problem hiding this comment.
that's a mistake, I'll fix if I touch the PR
|
ACK 131bed1 |
| #include <policy/ephemeral_policy.h> | ||
| #include <policy/policy.h> | ||
|
|
||
| bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate) |
There was a problem hiding this comment.
Nit: This function is independent of the ephemeral nature and is generic enough to lie outside this file, near where IsDust() is.
There was a problem hiding this comment.
In d5c564a
related: the code could be simplified a bit (imo) by changing HasDust to GetDust, which would require to move it to policy.h
git diff on d5c564a
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..6066d9b3ac 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,15 +5,10 @@
#include <policy/ephemeral_policy.h>
#include <policy/policy.h>
-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
-{
- return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
-}
-
bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
{
// We never want to give incentives to mine this transaction alone
- if ((base_fee != 0 || mod_fee != 0) && HasDust(tx, dust_relay_rate)) {
+ if ((base_fee != 0 || mod_fee != 0) && !GetDust(*tx, dust_relay_rate).empty()) {
return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0-fee");
}
@@ -52,11 +47,8 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
// Check for dust on parents
if (parent_ref) {
- for (uint32_t out_index = 0; out_index < parent_ref->vout.size(); out_index++) {
- const auto& tx_output = parent_ref->vout[out_index];
- if (IsDust(tx_output, dust_relay_rate)) {
- unspent_parent_dust.insert(COutPoint(parent_txid, out_index));
- }
+ for (const auto& out_index : GetDust(*parent_ref, dust_relay_rate)) {
+ unspent_parent_dust.insert(COutPoint{parent_txid, out_index});
}
}
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
index 26140f9a02..98f40d38ff 100644
--- a/src/policy/ephemeral_policy.h
+++ b/src/policy/ephemeral_policy.h
@@ -34,9 +34,6 @@
* are the only way to bring fees.
*/
-/** Returns true if transaction contains dust */
-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate);
-
/* All the following checks are only called if standardness rules are being applied. */
/** Must be called for each transaction once transaction fees are known.
diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
index 21c35af5cc..ed33692823 100644
--- a/src/policy/policy.cpp
+++ b/src/policy/policy.cpp
@@ -67,6 +67,15 @@ bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFeeIn)
return (txout.nValue < GetDustThreshold(txout, dustRelayFeeIn));
}
+std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate)
+{
+ std::vector<uint32_t> dust_outputs;
+ for (uint32_t i{0}; i < tx.vout.size(); ++i) {
+ if (IsDust(tx.vout[i], dust_relay_rate)) dust_outputs.push_back(i);
+ }
+ return dust_outputs;
+}
+
bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_datacarrier_bytes, TxoutType& whichType)
{
std::vector<std::vector<unsigned char> > vSolutions;
@@ -129,7 +138,6 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
}
unsigned int nDataOut = 0;
- unsigned int num_dust_outputs{0};
TxoutType whichType;
for (const CTxOut& txout : tx.vout) {
if (!::IsStandard(txout.scriptPubKey, max_datacarrier_bytes, whichType)) {
@@ -142,13 +150,11 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
else if ((whichType == TxoutType::MULTISIG) && (!permit_bare_multisig)) {
reason = "bare-multisig";
return false;
- } else if (IsDust(txout, dust_relay_fee)) {
- num_dust_outputs++;
}
}
// Only MAX_DUST_OUTPUTS_PER_TX dust is permitted(on otherwise valid ephemeral dust)
- if (num_dust_outputs > MAX_DUST_OUTPUTS_PER_TX) {
+ if (GetDust(tx, dust_relay_fee).size() > MAX_DUST_OUTPUTS_PER_TX) {
reason = "dust";
return false;
}
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 0488f8dbee..9e36d3f610 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -129,6 +129,9 @@ CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFee);
bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFee);
+/** Get the vout index numbers of all dust outputs */
+std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate);
+
bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_datacarrier_bytes, TxoutType& whichType);
That said, the current code is fine too, and I suspect you'll find this too big of a change for a PR of this size that otherwise seems close to merge.
| bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state) | ||
| { | ||
| // We never want to give incentives to mine this transaction alone | ||
| if ((base_fee != 0 || mod_fee != 0) && HasDust(tx, dust_relay_rate)) { | ||
| return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0-fee"); | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Could rename it PreCheckValidEphemeralTx to signify that it doesn't perform full validation.
| unspent_parent_dust.insert(COutPoint(parent_txid, out_index)); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| return true; | ||
| } | ||
|
|
||
| std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool) |
There was a problem hiding this comment.
Overall this is a pretty neat function that I enjoyed going through!
| // Only MAX_DUST_OUTPUTS_PER_TX dust is permitted(on otherwise valid ephemeral dust) | ||
| if (num_dust_outputs > MAX_DUST_OUTPUTS_PER_TX) { | ||
| reason = "dust"; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yes, IsStandardTx check is very early in PreChecks, not requiring access to any utxo information. including fees.
| /** | ||
| * Maximum number of ephemeral dust outputs allowed. | ||
| */ | ||
| static constexpr unsigned int MAX_DUST_OUTPUTS_PER_TX{1}; |
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
Can change to MAX_EPHEMERAL_DUST_OUTPUTS_PER_TX if I touch things
| // Non-0 fee dust transactions are not allowed for entry, and modification not allowed afterwards | ||
| const auto& tx = mempool.get(hash); | ||
| if (tx && HasDust(tx, mempool.m_opts.dust_relay_feerate)) { | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is not supported for transactions with dust outputs."); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is as intended. We don't allow priority on anything with dust, no matter how many non-zero dust outputs.
| for (const auto& tx_input : tx->vin) { | ||
| const Txid& parent_txid{tx_input.prevout.hash}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
IsStandardTxhas been changed. It's notTX_NOT_STANDARD. It needs to be 0 fee to passCheckValidEphemeralTx, and then it should fail on feerate forTX_RECONSIDERABLE. That's why the parent can be picked up by the package relay logic. Child also needs to passCheckEphemeralSpends, but otherwise neatly fits into 1p1c. - It's nice that
CheckValidEphemeralTxcomes beforeTX_RECONSIDERABLE, so we don't waste 1p1c cycles. But it's not strictly necessary to ensure an ephemeral tx is eligible for reconsideration.
| @@ -71,9 +71,39 @@ def test_dust_output(self, node: TestNode, dust_relay_fee: Decimal, | |||
| # finally send the transaction to avoid running out of MiniWallet UTXOs | |||
| self.wallet.sendrawtransaction(from_node=node, tx_hex=tx_good_hex) | |||
|
|
|||
| def test_dustrelay(self): | |||
There was a problem hiding this comment.
191ca05 nit: maybe test_dustrelayfee_zero or something
| } | ||
|
|
||
| for (const auto& tx : package) { | ||
| Txid txid = tx->GetHash(); |
There was a problem hiding this comment.
nit: const reference would be better
There was a problem hiding this comment.
nit: txid is only used once in the return statement at the bottom of the loop, so could skip creating this variable...
It seems to be introduced in response to #30239 (comment) - arguably we should probably tolerate GetHash() until the day it is renamed/aliased to GetTxid(). Lifetime is unnecessarily long as well.
|
|
||
| processed_parent_set.insert(parent_txid); | ||
| } | ||
|
|
There was a problem hiding this comment.
Could speed up the common case a bit by checking unspent_parent_dust.empty() here, but perhaps not significantly
| TestMemPoolEntryHelper entry; | ||
| CTxMemPool::setEntries empty_ancestors; | ||
|
|
||
| CFeeRate minrelay(1000); |
There was a problem hiding this comment.
Question, why minrelay instead of DUST_RELAY_TX_FEE?
| // Spend dust from one but not another is ok, as long as second grandparent has no child | ||
| BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, dust_spend}, minrelay, pool).has_value()); | ||
|
|
||
| auto dust_non_spend_both_parents = make_tx({COutPoint{dust_txid, dust_index}, COutPoint{dust_txid_2, dust_index - 1}}, /*version=*/2); |
There was a problem hiding this comment.
was this supposed to spend the non-dust from dust_txid 1?
There was a problem hiding this comment.
I think it's correct, spending dust from first dusty tx but not second
| // Add first grandparent to mempool and fetch entry | ||
| pool.addUnchecked(entry.FromTx(grandparent_tx_1)); | ||
|
|
||
| // Ignores ancestors that aren't direct parents |
There was a problem hiding this comment.
It also doesn't know about ancestors, given that the parent isn't given, right?
There was a problem hiding this comment.
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
| res = self.nodes[0].submitpackage([dusty_tx["hex"] for dusty_tx in dusty_txs] + [sweep_tx["hex"]]) | ||
| assert_equal(res['package_msg'], "success") | ||
| assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"] for dusty_tx in dusty_txs] + [sweep_tx["tx"], cancel_sweep["tx"]]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
could be worth a comment
| self.wallet.rescan_utxos() | ||
| assert_equal(self.nodes[0].getrawmempool(), []) | ||
|
|
||
| # Other topology tests require relaxation of submitpackage topology |
There was a problem hiding this comment.
I think "other topology" is a little under-specified here, could be helpful to name what tests you think could be added
| self.sync_all() | ||
|
|
||
| # N.B. this extra_args can be removed post cluster mempool | ||
| def test_free_relay(self): |
There was a problem hiding this comment.
nit: test_no_minrelay_fee could be better
| // Non-0 fee dust transactions are not allowed for entry, and modification not allowed afterwards | ||
| const auto& tx = mempool.get(hash); | ||
| if (tx && HasDust(tx, mempool.m_opts.dust_relay_feerate)) { | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is not supported for transactions with dust outputs."); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
alternatively could just check mempool.m_opts.require_standard as well?
There was a problem hiding this comment.
Code reviewed up until most of aa7c90a.
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.
| assert_equal(self.nodes[0].getrawmempool(), []) | ||
|
|
||
| # Double dust, both unspent, with fees. Would have failed individual checks. | ||
| # Dust is 1 satoshi create_self_transfer_multi disallows 0 |
There was a problem hiding this comment.
nit: Missing word
| # Dust is 1 satoshi create_self_transfer_multi disallows 0 | |
| # Dust is 1 satoshi since create_self_transfer_multi disallows 0 |
| dusty_tx = self.wallet.create_self_transfer_multi(fee_per_output=1000, amount_per_output=1, num_outputs=2) | ||
| dust_txid = self.nodes[0].sendrawtransaction(hexstring=dusty_tx["hex"], maxfeerate=0) |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| for (const auto& tx : package) { | ||
| Txid txid = tx->GetHash(); |
There was a problem hiding this comment.
nit: txid is only used once in the return statement at the bottom of the loop, so could skip creating this variable...
It seems to be introduced in response to #30239 (comment) - arguably we should probably tolerate GetHash() until the day it is renamed/aliased to GetTxid(). Lifetime is unnecessarily long as well.
| for (const auto& tx_input : tx->vin) { | ||
| const Txid& parent_txid{tx_input.prevout.hash}; | ||
| // Skip parents we've already checked dust for | ||
| if (processed_parent_set.contains(parent_txid)) continue; |
There was a problem hiding this comment.
nit: Since we unconditionally insert at the end of the loop, we might as well insert up here instead and check if it already existed in the set.
| if (processed_parent_set.contains(parent_txid)) continue; | |
| if (!processed_parent_set.insert(parent_txid).second) continue; |
| if (tx && HasDust(tx, mempool.m_opts.dust_relay_feerate)) { | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is not supported for transactions with dust outputs."); | ||
| } |
There was a problem hiding this comment.
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 d147d92 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).
There was a problem hiding this comment.
I independently came to the same conclusion, moving my review comment here:
I feel like d147d92 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?
| # We aren't checking spending, allow it in with no fee | ||
| self.restart_node(0, extra_args=["-minrelaytxfee=0"]) | ||
| self.restart_node(1, extra_args=["-minrelaytxfee=0"]) |
There was a problem hiding this comment.
Could add a comment about why -dustrelayfee is not a concern here?
| # Valid sweep we will RBF incorrectly by not spending dust as well | ||
| sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=dusty_tx["new_utxos"], version=3) | ||
| self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]]) | ||
| assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]]) | ||
|
|
||
| # Doesn't spend in-mempool dust output from parent | ||
| unspent_sweep_tx = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=[dusty_tx["new_utxos"][0]], version=3) | ||
| assert_greater_than(unspent_sweep_tx["fee"], sweep_tx["fee"]) | ||
| res = self.nodes[0].submitpackage([dusty_tx["hex"], unspent_sweep_tx["hex"]]) | ||
| 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") | ||
| 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"]) | ||
| assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]]) | ||
|
|
||
| # Spend works with dust spent | ||
| sweep_tx_2 = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=dusty_tx["new_utxos"], version=3) | ||
| assert sweep_tx["hex"] != sweep_tx_2["hex"] | ||
| res = self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx_2["hex"]]) | ||
| assert_equal(res["package_msg"], "success") |
There was a problem hiding this comment.
Slightly easier to follow comments?
| # Valid sweep we will RBF incorrectly by not spending dust as well | |
| sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=dusty_tx["new_utxos"], version=3) | |
| self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]]) | |
| assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]]) | |
| # Doesn't spend in-mempool dust output from parent | |
| unspent_sweep_tx = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=[dusty_tx["new_utxos"][0]], version=3) | |
| assert_greater_than(unspent_sweep_tx["fee"], sweep_tx["fee"]) | |
| res = self.nodes[0].submitpackage([dusty_tx["hex"], unspent_sweep_tx["hex"]]) | |
| 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") | |
| 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"]) | |
| assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]]) | |
| # Spend works with dust spent | |
| sweep_tx_2 = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=dusty_tx["new_utxos"], version=3) | |
| assert sweep_tx["hex"] != sweep_tx_2["hex"] | |
| res = self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx_2["hex"]]) | |
| assert_equal(res["package_msg"], "success") | |
| # Setup valid sweep in mempool | |
| sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=dusty_tx["new_utxos"], version=3) | |
| self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]]) | |
| assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]]) | |
| # RBF-attempt fails when not spending in-mempool dust output from parent | |
| unspent_sweep_tx = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=[dusty_tx["new_utxos"][0]], version=3) | |
| assert_greater_than(unspent_sweep_tx["fee"], sweep_tx["fee"]) | |
| res = self.nodes[0].submitpackage([dusty_tx["hex"], unspent_sweep_tx["hex"]]) | |
| 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") | |
| 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"]) | |
| assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]]) | |
| # RBF works when dust is spent | |
| sweep_tx_2 = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=dusty_tx["new_utxos"], version=3) | |
| assert sweep_tx["hex"] != sweep_tx_2["hex"] | |
| res = self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx_2["hex"]]) | |
| assert_equal(res["package_msg"], "success") |
| dusty_tx = self.wallet.create_self_transfer_multi( | ||
| fee_per_output=0, | ||
| version=3 | ||
| ) |
There was a problem hiding this comment.
Why the sudden change in style?
| dusty_tx = self.wallet.create_self_transfer_multi( | |
| fee_per_output=0, | |
| version=3 | |
| ) | |
| dusty_tx = self.wallet.create_self_transfer_multi(fee_per_output=0, version=3) |
| @@ -21,6 +21,20 @@ | |||
|
|
|||
| ORPHAN_TX_EXPIRE_TIME = 1200 | |||
|
|
|||
| def assert_mempool_contents(test_framework, node, expected=None, sync=True): | |||
There was a problem hiding this comment.
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.
| block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [dusty_tx["hex"]]) | ||
| self.nodes[0].invalidateblock(block_res["hash"]) |
There was a problem hiding this comment.
nit: Could validate/document that tx made it into the block to be sure, before invalidating.
| block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [dusty_tx["hex"]]) | |
| self.nodes[0].invalidateblock(block_res["hash"]) | |
| block_res = self.nodes[0].rpc.generateblock(self.wallet.get_address(), [dusty_tx["hex"]]) | |
| assert_equal(self.nodes[0].getrawtransaction(dusty_tx["txid"], blockhash=block_res["hash"]), dusty_tx["hex"]) | |
| self.nodes[0].invalidateblock(block_res["hash"]) |
| # Double dust, both unspent, with fees. Would have failed individual checks. | ||
| # Dust is 1 satoshi create_self_transfer_multi disallows 0 |
There was a problem hiding this comment.
In 2873351:
nit: I would either remove these docstrings, or fix up the grammar, I spent way more time trying to parse these (and failed) than just reading the code
# Create two dust outputs. Both are unspent, have fees, and would have failed individual checks.
# The amount is 1 satoshi because create_self_transfer_multi disallows 0.
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #include <policy/ephemeral_policy.h> |
There was a problem hiding this comment.
nit: includes are not quite up to date (in both files)
git diff on d5c564a to make iwyu happy
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..3a9a3fa530 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,6 +5,22 @@
#include <policy/ephemeral_policy.h>
#include <policy/policy.h>
+#include <consensus/validation.h>
+#include <policy/feerate.h>
+#include <policy/packages.h>
+#include <primitives/transaction.h>
+#include <txmempool.h>
+#include <util/check.h>
+#include <util/hasher.h>
+
+#include <algorithm>
+#include <cstdint>
+#include <map>
+#include <memory>
+#include <unordered_set>
+#include <utility>
+#include <vector>
+
bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
{
return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
index 26140f9a02..742cb30280 100644
--- a/src/policy/ephemeral_policy.h
+++ b/src/policy/ephemeral_policy.h
@@ -5,10 +5,15 @@
#ifndef BITCOIN_POLICY_EPHEMERAL_POLICY_H
#define BITCOIN_POLICY_EPHEMERAL_POLICY_H
+#include <consensus/amount.h>
#include <policy/packages.h>
-#include <policy/policy.h>
#include <primitives/transaction.h>
-#include <txmempool.h>
+
+#include <optional>
+
+class CFeeRate;
+class CTxMemPool;
+class TxValidationState;
/** These utility functions ensure that ephemeral dust is safely
* created and spent without unduly risking them entering the utxo
|
|
||
| bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate) | ||
| { | ||
| return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); }); |
There was a problem hiding this comment.
In d5c564a:
nit: could make this a bit nicer with std::ranges:
git diff on d5c564a
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..569f9a95b5 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -7,7 +7,7 @@
bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
{
- return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
+ return std::ranges::any_of(tx->vout, [&](const auto& output) { return IsDust(output, dust_relay_rate); });
}
bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
@@ -22,7 +22,7 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
{
- if (!Assume(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}))) {
+ if (!Assume(std::ranges::all_of(package, [](const auto& tx){return tx != nullptr;}))) {
// Bail out of spend checks if caller gave us an invalid package
return std::nullopt;
}
| * where parents are either in the mempool or in the package itself. | ||
| * The function returns std::nullopt if all dust is properly spent, or the txid of the violating child spend. | ||
| */ | ||
| std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool); |
There was a problem hiding this comment.
I think returning a falsy value for a successful check is an antipattern. This would be nicely addressed by #25665, but until that's merged I think I'd prefer returning a std::pair<bool, std::optional<Txid>>, even if it's a bit more verbose:
git diff on d5c564a
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..10a8ab1938 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,6 +5,8 @@
#include <policy/ephemeral_policy.h>
#include <policy/policy.h>
+#include <utility>
+
bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
{
return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
@@ -20,11 +22,11 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
return true;
}
-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
+std::pair<bool, std::optional<Txid>> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
{
if (!Assume(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}))) {
// Bail out of spend checks if caller gave us an invalid package
- return std::nullopt;
+ return {true, std::nullopt};
}
std::map<Txid, CTransactionRef> map_txid_ref;
@@ -70,9 +72,9 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
}
if (!unspent_parent_dust.empty()) {
- return txid;
+ return {false, txid};
}
}
- return std::nullopt;
+ return {true, std::nullopt};
}
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
index 26140f9a02..7235dd7500 100644
--- a/src/policy/ephemeral_policy.h
+++ b/src/policy/ephemeral_policy.h
@@ -10,6 +10,8 @@
#include <primitives/transaction.h>
#include <txmempool.h>
+#include <utility>
+
/** These utility functions ensure that ephemeral dust is safely
* created and spent without unduly risking them entering the utxo
* set.
@@ -50,6 +52,6 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
* where parents are either in the mempool or in the package itself.
* The function returns std::nullopt if all dust is properly spent, or the txid of the violating child spend.
*/
-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
+std::pair<bool, std::optional<Txid>> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
#endif // BITCOIN_POLICY_EPHEMERAL_POLICY_H
diff --git a/src/validation.cpp b/src/validation.cpp
index 2f3e7d61a8..94cbdf2766 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1456,11 +1456,11 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
}
if (m_pool.m_opts.require_standard) {
- if (const auto ephemeral_violation{CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
- const Txid& txid = ephemeral_violation.value();
- Assume(txid == ptx->GetHash());
+ const auto& [is_ephemeral, offending_txid] = CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool);
+ if (!is_ephemeral) {
+ Assume(offending_txid.value() == ptx->GetHash());
ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
- strprintf("tx %s did not spend parent's ephemeral dust", txid.ToString()));
+ strprintf("tx %s did not spend parent's ephemeral dust", offending_txid.value().ToString()));
return MempoolAcceptResult::Failure(ws.m_state);
}
}
@@ -1605,13 +1605,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
// Now that we've bounded the resulting possible ancestry count, check package for dust spends
if (m_pool.m_opts.require_standard) {
- if (const auto ephemeral_violation{CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
- const Txid& child_txid = ephemeral_violation.value();
+ const auto& [is_ephemeral, offending_txid] = CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool);
+ if (!is_ephemeral) {
TxValidationState child_state;
child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
- strprintf("tx %s did not spend parent's ephemeral dust", child_txid.ToString()));
+ strprintf("tx %s did not spend parent's ephemeral dust", offending_txid.value().ToString()));
package_state.Invalid(PackageValidationResult::PCKG_TX, "unspent-dust");
- results.emplace(child_txid, MempoolAcceptResult::Failure(child_state));
+ results.emplace(offending_txid.value(), MempoolAcceptResult::Failure(child_state));
return PackageMempoolAcceptResult(package_state, std::move(results));
}
}
There was a problem hiding this comment.
Actually, returning a bool and adding a TxValidationState& out-parameter would simultaneously avoid the anti-pattern and reduce code duplication:
git diff on d5c564a
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..94868f8fcb 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -20,11 +20,11 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
return true;
}
-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
+bool CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool, TxValidationState& child_state)
{
if (!Assume(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}))) {
// Bail out of spend checks if caller gave us an invalid package
- return std::nullopt;
+ return true;
}
std::map<Txid, CTransactionRef> map_txid_ref;
@@ -70,9 +70,11 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
}
if (!unspent_parent_dust.empty()) {
- return txid;
+ child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
+ strprintf("tx %s did not spend parent's ephemeral dust", txid.ToString()));
+ return false;
}
}
- return std::nullopt;
+ return true;
}
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
index 26140f9a02..bf2c9691a7 100644
--- a/src/policy/ephemeral_policy.h
+++ b/src/policy/ephemeral_policy.h
@@ -48,8 +48,8 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
/** Must be called for each transaction(package) if any dust is in the package.
* Checks that each transaction's parents have their dust spent by the child,
* where parents are either in the mempool or in the package itself.
- * The function returns std::nullopt if all dust is properly spent, or the txid of the violating child spend.
+ * The function returns true if all dust is properly spent.
*/
-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
+bool CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool, TxValidationState& child_state);
#endif // BITCOIN_POLICY_EPHEMERAL_POLICY_H
diff --git a/src/validation.cpp b/src/validation.cpp
index 2f3e7d61a8..f6d05956ee 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1456,11 +1456,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
}
if (m_pool.m_opts.require_standard) {
- if (const auto ephemeral_violation{CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
- const Txid& txid = ephemeral_violation.value();
- Assume(txid == ptx->GetHash());
- ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
- strprintf("tx %s did not spend parent's ephemeral dust", txid.ToString()));
+ if (!CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool, ws.m_state)) {
return MempoolAcceptResult::Failure(ws.m_state);
}
}
@@ -1605,13 +1601,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
// Now that we've bounded the resulting possible ancestry count, check package for dust spends
if (m_pool.m_opts.require_standard) {
- if (const auto ephemeral_violation{CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
- const Txid& child_txid = ephemeral_violation.value();
- TxValidationState child_state;
- child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
- strprintf("tx %s did not spend parent's ephemeral dust", child_txid.ToString()));
+ if (TxValidationState child_state; !CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool, child_state)) {
package_state.Invalid(PackageValidationResult::PCKG_TX, "unspent-dust");
- results.emplace(child_txid, MempoolAcceptResult::Failure(child_state));
+ results.emplace(txns.back()->GetHash(), MempoolAcceptResult::Failure(child_state));
return PackageMempoolAcceptResult(package_state, std::move(results));
}
}
| @@ -142,11 +143,16 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat | |||
| reason = "bare-multisig"; | |||
| return false; | |||
| } else if (IsDust(txout, dust_relay_fee)) { | |||
| reason = "dust"; | |||
| return false; | |||
| num_dust_outputs++; | |||
There was a problem hiding this comment.
In d5c564a
developer-notes-nit: ++i is preferred over i++.
| #include <policy/ephemeral_policy.h> | ||
| #include <policy/policy.h> | ||
|
|
||
| bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate) |
There was a problem hiding this comment.
In d5c564a
related: the code could be simplified a bit (imo) by changing HasDust to GetDust, which would require to move it to policy.h
git diff on d5c564a
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..6066d9b3ac 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,15 +5,10 @@
#include <policy/ephemeral_policy.h>
#include <policy/policy.h>
-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
-{
- return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
-}
-
bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
{
// We never want to give incentives to mine this transaction alone
- if ((base_fee != 0 || mod_fee != 0) && HasDust(tx, dust_relay_rate)) {
+ if ((base_fee != 0 || mod_fee != 0) && !GetDust(*tx, dust_relay_rate).empty()) {
return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0-fee");
}
@@ -52,11 +47,8 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
// Check for dust on parents
if (parent_ref) {
- for (uint32_t out_index = 0; out_index < parent_ref->vout.size(); out_index++) {
- const auto& tx_output = parent_ref->vout[out_index];
- if (IsDust(tx_output, dust_relay_rate)) {
- unspent_parent_dust.insert(COutPoint(parent_txid, out_index));
- }
+ for (const auto& out_index : GetDust(*parent_ref, dust_relay_rate)) {
+ unspent_parent_dust.insert(COutPoint{parent_txid, out_index});
}
}
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
index 26140f9a02..98f40d38ff 100644
--- a/src/policy/ephemeral_policy.h
+++ b/src/policy/ephemeral_policy.h
@@ -34,9 +34,6 @@
* are the only way to bring fees.
*/
-/** Returns true if transaction contains dust */
-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate);
-
/* All the following checks are only called if standardness rules are being applied. */
/** Must be called for each transaction once transaction fees are known.
diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
index 21c35af5cc..ed33692823 100644
--- a/src/policy/policy.cpp
+++ b/src/policy/policy.cpp
@@ -67,6 +67,15 @@ bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFeeIn)
return (txout.nValue < GetDustThreshold(txout, dustRelayFeeIn));
}
+std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate)
+{
+ std::vector<uint32_t> dust_outputs;
+ for (uint32_t i{0}; i < tx.vout.size(); ++i) {
+ if (IsDust(tx.vout[i], dust_relay_rate)) dust_outputs.push_back(i);
+ }
+ return dust_outputs;
+}
+
bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_datacarrier_bytes, TxoutType& whichType)
{
std::vector<std::vector<unsigned char> > vSolutions;
@@ -129,7 +138,6 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
}
unsigned int nDataOut = 0;
- unsigned int num_dust_outputs{0};
TxoutType whichType;
for (const CTxOut& txout : tx.vout) {
if (!::IsStandard(txout.scriptPubKey, max_datacarrier_bytes, whichType)) {
@@ -142,13 +150,11 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
else if ((whichType == TxoutType::MULTISIG) && (!permit_bare_multisig)) {
reason = "bare-multisig";
return false;
- } else if (IsDust(txout, dust_relay_fee)) {
- num_dust_outputs++;
}
}
// Only MAX_DUST_OUTPUTS_PER_TX dust is permitted(on otherwise valid ephemeral dust)
- if (num_dust_outputs > MAX_DUST_OUTPUTS_PER_TX) {
+ if (GetDust(tx, dust_relay_fee).size() > MAX_DUST_OUTPUTS_PER_TX) {
reason = "dust";
return false;
}
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 0488f8dbee..9e36d3f610 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -129,6 +129,9 @@ CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFee);
bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFee);
+/** Get the vout index numbers of all dust outputs */
+std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate);
+
bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_datacarrier_bytes, TxoutType& whichType);
That said, the current code is fine too, and I suspect you'll find this too big of a change for a PR of this size that otherwise seems close to merge.
| // Add dust output to take dust slot, still standard! | ||
| t.vout.emplace_back(0, t.vout[0].scriptPubKey); | ||
| CheckIsStandard(t); |
There was a problem hiding this comment.
In d5c564a
Since MAX_DUST_OUTPUTS_PER_TX is parameterized, it would make the tests more robust to dynamically add dust outputs too?
git diff on d5c564a
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 0488f8dbee..e023c99560 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -80,7 +80,7 @@ static constexpr unsigned int EXTRA_DESCENDANT_TX_SIZE_LIMIT{10000};
/**
* Maximum number of ephemeral dust outputs allowed.
*/
-static constexpr unsigned int MAX_DUST_OUTPUTS_PER_TX{1};
+static constexpr unsigned int MAX_DUST_OUTPUTS_PER_TX{4};
/**
* Mandatory script verification flags that all new transactions must comply with for
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
index 3e4c085c0e..f38c015f6d 100644
--- a/src/test/transaction_tests.cpp
+++ b/src/test/transaction_tests.cpp
@@ -814,9 +814,11 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
CAmount nDustThreshold = 182 * g_dust.GetFeePerK() / 1000;
BOOST_CHECK_EQUAL(nDustThreshold, 546);
- // Add dust output to take dust slot, still standard!
- t.vout.emplace_back(0, t.vout[0].scriptPubKey);
- CheckIsStandard(t);
+ // Add dust outputs up to allowed maximum, still standard!
+ for (size_t i{0}; i < MAX_DUST_OUTPUTS_PER_TX; ++i) {
+ t.vout.emplace_back(0, t.vout[0].scriptPubKey);
+ CheckIsStandard(t);
+ }
// dust:
t.vout[0].nValue = nDustThreshold - 1;
@@ -974,9 +976,9 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
CheckIsNotStandard(t, "bare-multisig");
g_bare_multi = DEFAULT_PERMIT_BAREMULTISIG;
- // Add dust output to take dust slot
+ // Add dust outputs up to allowed maximum
assert(t.vout.size() == 1);
- t.vout.emplace_back(0, t.vout[0].scriptPubKey);
+ t.vout.insert(t.vout.end(), MAX_DUST_OUTPUTS_PER_TX, {0, t.vout[0].scriptPubKey});
// Check compressed P2PK outputs dust threshold (must have leading 02 or 03)
t.vout[0].scriptPubKey = CScript() << std::vector<unsigned char>(33, 0x02) << OP_CHECKSIG;
| return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); }); | ||
| } | ||
|
|
||
| bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state) |
There was a problem hiding this comment.
Is there a reason for these to be const CTransactionRef& instead of const CTransaction&? The latter would avoid nullptr issues as well as improve reusability.
git diff on d5c564a
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..c9f1ba076d 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,12 +5,12 @@
#include <policy/ephemeral_policy.h>
#include <policy/policy.h>
-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
+bool HasDust(const CTransaction& tx, CFeeRate dust_relay_rate)
{
- return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
+ return std::any_of(tx.vout.cbegin(), tx.vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
}
-bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
+bool CheckValidEphemeralTx(const CTransaction& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
{
// We never want to give incentives to mine this transaction alone
if ((base_fee != 0 || mod_fee != 0) && HasDust(tx, dust_relay_rate)) {
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
index 26140f9a02..10a5edf337 100644
--- a/src/policy/ephemeral_policy.h
+++ b/src/policy/ephemeral_policy.h
@@ -35,7 +35,7 @@
*/
/** Returns true if transaction contains dust */
-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate);
+bool HasDust(const CTransaction& tx, CFeeRate dust_relay_rate);
/* All the following checks are only called if standardness rules are being applied. */
@@ -43,7 +43,7 @@ bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate);
* Does context-less checks about a single transaction.
* Returns false if the fee is non-zero and dust exists, populating state. True otherwise.
*/
-bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state);
+bool CheckValidEphemeralTx(const CTransaction& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state);
/** Must be called for each transaction(package) if any dust is in the package.
* Checks that each transaction's parents have their dust spent by the child,
diff --git a/src/validation.cpp b/src/validation.cpp
index 2f3e7d61a8..8387a7f971 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -930,7 +930,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
// Enforces 0-fee for dust transactions, no incentive to be mined alone
if (m_pool.m_opts.require_standard) {
- if (!CheckValidEphemeralTx(ptx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, ws.m_modified_fees, state)) {
+ if (!CheckValidEphemeralTx(tx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, ws.m_modified_fees, state)) {
return false; // state filled in by CheckValidEphemeralTx
}
}
| // Enforces 0-fee for dust transactions, no incentive to be mined alone | ||
| if (m_pool.m_opts.require_standard) { | ||
| if (!CheckValidEphemeralTx(ptx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, ws.m_modified_fees, state)) { | ||
| return false; // state filled in by CheckValidEphemeralTx |
There was a problem hiding this comment.
I'd prefer actually asserting this instead of just documenting it (unsure if we prefer !IsValid() or IsInvalid() here, the latter not capturing IsError()).
git diff on d5c564a
diff --git a/src/validation.cpp b/src/validation.cpp
index 2f3e7d61a8..ac619fa385 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -931,7 +931,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
// Enforces 0-fee for dust transactions, no incentive to be mined alone
if (m_pool.m_opts.require_standard) {
if (!CheckValidEphemeralTx(ptx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, ws.m_modified_fees, state)) {
- return false; // state filled in by CheckValidEphemeralTx
+ Assert(!state.IsValid());
+ return false;
}
}
| if (tx && HasDust(tx, mempool.m_opts.dust_relay_feerate)) { | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is not supported for transactions with dust outputs."); | ||
| } |
There was a problem hiding this comment.
I independently came to the same conclusion, moving my review comment here:
I feel like d147d92 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?
There was a problem hiding this comment.
Concept ACK 131bed1
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.
| self.nodes[0].invalidateblock(block_res["hash"]) | ||
| assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"]], sync=False) | ||
|
|
||
| # Also should happen if dust is swept |
There was a problem hiding this comment.
"Also"? But in this case the sweep-transaction survives the reorg, unlike the case above.
| assert_equal(res['package_msg'], "success") | ||
| assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"] for dusty_tx in dusty_txs] + [sweep_tx["tx"], cancel_sweep["tx"]]) | ||
|
|
||
| self.generate(self.nodes[0], 25) |
|
|
||
| chainstate.SetMempool(&tx_pool); | ||
|
|
||
| LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300) |
There was a problem hiding this comment.
(Just curious - this is a very common pattern, why do we leave it up to the fuzz engine instead of just doing:
| LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300) | |
| 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).)
| std::optional<COutPoint> outpoint_to_rbf{GetChildEvictingPrevout(tx_pool)}; | ||
| bool should_rbf_eph_spend = outpoint_to_rbf && fuzzed_data_provider.ConsumeBool(); |
There was a problem hiding this comment.
nit: Reduce the number of variables:
| std::optional<COutPoint> outpoint_to_rbf{GetChildEvictingPrevout(tx_pool)}; | |
| bool should_rbf_eph_spend = outpoint_to_rbf && fuzzed_data_provider.ConsumeBool(); | |
| std::optional<COutPoint> outpoint_to_rbf{fuzzed_data_provider.ConsumeBool() ? GetChildEvictingPrevout(tx_pool) : std::nullopt}; |
| // Create transaction to add to the mempool | ||
| const CTransactionRef tx = [&] { | ||
| CMutableTransaction tx_mut; | ||
| tx_mut.version = CTransaction::CURRENT_VERSION; |
There was a problem hiding this comment.
Only exercising v2 - maybe you could describe the reasoning behind the test at the top of fuzz target?
| } | ||
| for (auto i{0}; i < 3; ++i) { | ||
| mtx.vout[i].scriptPubKey = CScript() << OP_TRUE; | ||
| mtx.vout[i].nValue = (i == 2) ? 0 : 10000; |
There was a problem hiding this comment.
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?
| // We first start with nothing "in the mempool", using package checks | ||
|
|
||
| // Trivial single transaction with no dust | ||
| BOOST_CHECK(!CheckEphemeralSpends({dust_spend}, minrelay, pool).has_value()); |
There was a problem hiding this comment.
Could rely on optional's bool operator to decrease noise here and below:
| BOOST_CHECK(!CheckEphemeralSpends({dust_spend}, minrelay, pool).has_value()); | |
| BOOST_CHECK(!CheckEphemeralSpends({dust_spend}, minrelay, pool)); |
| auto dust_non_spend = make_tx({COutPoint{dust_txid, dust_index - 1}}, /*version=*/2); | ||
|
|
||
| // Child spending non-dust only from parent should be disallowed even if dust otherwise spent | ||
| BOOST_CHECK(CheckEphemeralSpends({grandparent_tx_1, dust_non_spend, dust_spend}, minrelay, pool).has_value()); |
There was a problem hiding this comment.
Might as well be explicit about expected txid here and elsewhere?
| BOOST_CHECK(CheckEphemeralSpends({grandparent_tx_1, dust_non_spend, dust_spend}, minrelay, pool).has_value()); | |
| BOOST_CHECK_EQUAL(CheckEphemeralSpends({grandparent_tx_1, dust_non_spend, dust_spend}, minrelay, pool), dust_non_spend->GetHash()); |
| } | ||
|
|
||
| // Tx with many outputs | ||
| CMutableTransaction tx1 = CMutableTransaction(); |
There was a problem hiding this comment.
Here and for tx2:
| CMutableTransaction tx1 = CMutableTransaction(); | |
| CMutableTransaction tx1; |
|
|
||
| bench.run([&]() NO_THREAD_SAFETY_ANALYSIS { | ||
|
|
||
| CheckEphemeralSpends({tx2_r}, /*dust_relay_rate=*/CFeeRate(iteration * COIN / 10), pool); |
There was a problem hiding this comment.
With the diff:
diff --git a/src/bench/mempool_ephemeral_spends.cpp b/src/bench/mempool_ephemeral_spends.cpp
index e867c61752..dc5e577caf 100644
--- a/src/bench/mempool_ephemeral_spends.cpp
+++ b/src/bench/mempool_ephemeral_spends.cpp
@@ -72,12 +72,16 @@ static void MempoolCheckEphemeralSpends(benchmark::Bench& bench)
AddTx(tx1_r, pool);
uint32_t iteration{0};
+ uint32_t failure{0};
bench.run([&]() NO_THREAD_SAFETY_ANALYSIS {
- CheckEphemeralSpends({tx2_r}, /*dust_relay_rate=*/CFeeRate(iteration * COIN / 10), pool);
+ if (CheckEphemeralSpends({tx2_r}, /*dust_relay_rate=*/CFeeRate(iteration * COIN / 10), pool))
+ ++failure;
iteration++;
});
+
+ printf("success: %d, failure: %d\n", iteration - failure, failure);
}
BENCHMARK(MempoolCheckEphemeralSpends, benchmark::PriorityLevel::HIGH);The result was:
₿ build/src/bench/bench_bitcoin -filter=MempoolCheckEphemeralSpends -min-time=30000
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 234,976.30 | 4,255.75 | 0.4% | 2,384,033.36 | 859,700.17 | 2.773 | 178,732.26 | 1.3% | 32.29 | `MempoolCheckEphemeralSpends`
success: 1, failure: 138329
Is the intention that we only succeed on iteration 0? Could maybe initialize the iteration to 1 and change the feerate expression, making sure it still succeeds at least once?
| assert_equal(self.nodes[0].getrawmempool(), [dust_txid]) | ||
|
|
||
| # Spends one dust along with fee input, leave other dust unspent to check ephemeral dust checks aren't being enforced | ||
| sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[self.wallet.get_utxo(), dusty_tx["new_utxos"][0]]) |
There was a problem hiding this comment.
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.
| /** Returns true if transaction contains dust */ | ||
| bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate); | ||
|
|
||
| /* All the following checks are only called if standardness rules are being applied. */ | ||
|
|
||
| /** Must be called for each transaction once transaction fees are known. | ||
| * Does context-less checks about a single transaction. | ||
| * Returns false if the fee is non-zero and dust exists, populating state. True otherwise. | ||
| */ | ||
| bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state); | ||
|
|
||
| /** Must be called for each transaction(package) if any dust is in the package. | ||
| * Checks that each transaction's parents have their dust spent by the child, | ||
| * where parents are either in the mempool or in the package itself. | ||
| * The function returns std::nullopt if all dust is properly spent, or the txid of the violating child spend. | ||
| */ | ||
| std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool); |
There was a problem hiding this comment.
nit: The comment blocks before each function could be updated to use doxygen commands. Could be left for follow-up.
Example:
/** Must be called for each transaction once transaction fees are known.
* Does context-less checks about a single transaction.
- * Returns false if the fee is non-zero and dust exists, populating state. True otherwise.
+ * @returns false if the fee is non-zero and dust exists, populating state. True otherwise.
*/
bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state);| /** Must be called for each transaction(package) if any dust is in the package. | ||
| * Checks that each transaction's parents have their dust spent by the child, | ||
| * where parents are either in the mempool or in the package itself. | ||
| * The function returns std::nullopt if all dust is properly spent, or the txid of the violating child spend. |
There was a problem hiding this comment.
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 (comment) could increase consistency.
| bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state) | ||
| { | ||
| // We never want to give incentives to mine this transaction alone | ||
| if ((base_fee != 0 || mod_fee != 0) && HasDust(tx, dust_relay_rate)) { |
There was a problem hiding this comment.
If it's decided to allow non-zero modified fee, then the latter half of the || might need to be adjusted here (#30239 (comment)).
| } | ||
| } | ||
|
|
||
| processed_parent_set.insert(parent_txid); |
There was a problem hiding this comment.
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.
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:
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: lightning/bolts#1171 (comment)
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.