It was calculated as if it needed a compressed pubkey + a DER encoded ECDSA signature.
Eventually i believe it would be good to prevent future overlooks by not applying the dust check to unknown witness versions and aborting if witness_version > current_version. Had a commit that starts doing that but removed it as it made this simple fix PR needlessly more complex.
41+ if (witnessversion == 0) {
42+ // P2WPKH spend, a compressed public key + a DER-encoded ECDSA signature
43+ nSize += (32 + 4 + 1 + (107 / WITNESS_SCALE_FACTOR) + 4);
44+ } else {
45+ // Taproot key spend, a single BIP341 signature
46+ nSize += (32 + 4 + 1 + (65 / WITNESS_SCALE_FACTOR) + 4);
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
954@@ -955,6 +955,15 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
955 BOOST_CHECK(!IsStandardTx(CTransaction(t), reason));
956 BOOST_CHECK_EQUAL(reason, "bare-multisig");
957 fIsBareMultisigStd = DEFAULT_PERMIT_BAREMULTISIG;
958+
959+ // Check Taproot outputs dust threshold
960+ t.vout[0].scriptPubKey = CScript() << OP_1 << ParseHex("72ad3fb702bf1a2111b09c2bf1e72f4f52d4ceb2acdcfcd0c63abf70a59c36d6");
You can just use a repeating hex character to allow for better compression.
0 t.vout[0].scriptPubKey = CScript() << OP_1 << ParseHex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
Also, would be nice to add a test for OP_16 (as you change this too in this pull) and OP_0, because it is missing.
Used repeating characters, tested for Segwit v0, and added test for future witness versions.
Awkward to test something that is almost certainly wrong, comforts me in the idea that we should refactor GetDustThreshold to loudly fail rather than returning an invalid value.
41+ if (witnessversion == 0) {
42+ // P2WPKH spend, a compressed public key + a DER-encoded ECDSA signature
43+ nSize += (32 + 4 + 1 + (107 / WITNESS_SCALE_FACTOR) + 4);
44+ } else {
45+ // Taproot key spend, a single BIP341 signature
46+ nSize += (32 + 4 + 1 + (66 / WITNESS_SCALE_FACTOR) + 4);
More of a meta-comment:
If GetDustThreshold attempts to calculate the cost at which it’s economical to spend an output, why is nSize initialized to GetSerializedSize(txout)? When spending the dust output, the size of the txout isn’t in the spending tx unless the spending tx also spends to the same output type. This heuristic/initialization doesn’t make sense to me because if we take P2WSH as an example (w/ dust limit of 330), 330 - fees < 330 it is impossible to spend to the same output type unless other inputs are added. It seems to me that if witness version non-zero is being changed, this could be a place to fix or clarify this calculation for witness version non-zero.
Concept ACK
Eventually i believe it would be good to prevent future overlooks by not applying the dust check to unknown witness versions and aborting if witness_version > current_version.
IIUC, you’re aiming to return IsDust() == true for any transaction with witness outputs superior to current version, wouldn’t this be a revert of #15846 ? As we don’t know what would be the minimal spent or we can pick-up an arbitrary amount e.g 1000 sats, and consider the effective dust threshold for future type will be lower. And as such minimize risks of policy tightening changes.
No, i’m aiming to do the opposite: not call IsDust for witness versions >1, effectively having IsDust == false for any witness version from the future. Since, as you mention it, we can’t know the size in advance. ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ Le mardi 24 août 2021 à 11:21 PM, Antoine Riard @.***> a écrit :
Concept ACK
Eventually i believe it would be good to prevent future overlooks by not applying the dust check to unknown witness versions and aborting if witness_version > current_version.
IIUC, you’re aiming to return IsDust() == true for any transaction with witness outputs superior to current version, wouldn’t this be a revert of #15846 ? As we don’t know what would be the minimal spent or we can pick-up an arbitrary amount e.g 1000 sats, and consider the effective dust threshold for future type will be lower. And as such minimize risks of policy tightening changes.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.
36@@ -37,7 +37,13 @@ CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFeeIn)
37 if (txout.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram)) {
38 // sum the sizes of the parts of a transaction input
39 // with 75% segwit discount applied to the script size.
40- nSize += (32 + 4 + 1 + (107 / WITNESS_SCALE_FACTOR) + 4);
41+ if (witnessversion == 0) {
42+ // P2WPKH spend, a compressed public key + a DER-encoded ECDSA signature
43+ nSize += (32 + 4 + 1 + (107 / WITNESS_SCALE_FACTOR) + 4);
44+ } else {
45+ // Taproot key spend, a single BIP341 signature
No, i’m aiming to do the opposite: not call IsDust for witness versions >1, effectively having IsDust == false for any witness version from the future.
Hmmmm so you could propagate transactions SegWit v2+ with nValue == 0 and as such freely write on the UTXO set? Unknown witness outputs are standard iirc.
Hmmmm so you could propagate transactions SegWit v2+ with nValue == 0 and as such freely write on the UTXO set? Unknown witness outputs are standard iirc.
You are right, i shouldn’t reply to Github comments at 11:21PM :p Maybe it could be made of a fixed value (this one, or another large one so we don’t tighten it when introducing a new version). Anyways i don’t intend to (address it in this PR and) modify the existing behaviour, just to refactor the code to prevent a future overlook.
It was calculated as if it needed a compressed pubkey + a DER encoded
ECDSA signature.
Note that this commit intentionally keeps the existing behaviour of
applying the latest witness program version computation to all newer
versions.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Code Review ACK 9155f6c
Maybe it could be made of a fixed value (this one, or another large one so we don’t tighten it when introducing a new version). Anyways i don’t intend to (address it in this PR and) modify the existing behaviour, just to refactor the code to prevent a future overlook.
Yes honestly I think we should revamp our approach w.r.t to dust outputs, from a hard non-propagating threshold to a feerate penalty approach, that way adjustable by L2 nodes at broadcast time in function of mempool congestion. Though need to work-out a design first and discussion better left to the ML :)
36@@ -37,7 +37,13 @@ CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFeeIn)
37 if (txout.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram)) {
38 // sum the sizes of the parts of a transaction input
39 // with 75% segwit discount applied to the script size.
40- nSize += (32 + 4 + 1 + (107 / WITNESS_SCALE_FACTOR) + 4);
41+ if (witnessversion == 0) {
Concept NACK –
I think we should either remove the dust limit entirely or not muck around with it.
Dust limits are not incentive compatible with miner preferences and if they are they can be configured by users individually.
That said, it’s CR-Ack for me for this code, which seems to be improving the dust nSize calculation (which should be independent of any actual fee policy, the code could be cleaned up to make it have a min somewhere later but we shouldn’t confuse byte counting with thresholds).
@MarcoFalke done in #22846
@TheBlueMatt i too am of the opinion that we should not decrease the dust thresholds, but i think that if we want to keep the same dust thresholds for Taproot outputs we should instead raise DUST_RELAY_TX_FEE instead of keeping the wrong computation. But this would unfortunately be a security issue for (some) L2s relying on it..
I don’t think the rationale of “dust threshold was calculated many years ago, let’s not fix it for a new output type unused on the network yet” holds. It was adapted for Segwit v0 and now Segwit v1 should be “penalized”? In any case, i think that even if we want to keep it larger for Taproot outputs than for Segwit v0 outputs we should document it instead of having the current “at least a 33-bytes pubkey and an ECDSA signature are required to spend” rationale for any Segwit output.
@TheBlueMatt i too am of the opinion that we should not decrease the dust thresholds, but i think that if we want to keep the same dust thresholds for Taproot outputs we should instead raise DUST_RELAY_TX_FEE instead of keeping the wrong computation. But this would unfortunately be a security issue for (some) L2s relying on it..
Indeed, I don’t think we can in any practical sense increase the dust threshold for deployed things, for exactly the reason you specify. I also agree with you that, ideally, we’d increase DUST_RELAY_TX_FEE, but given that we agree that we cannot, the only thing we can do is not make the problem worse for, which I’d argue this does.
I’m not really sure why “consistency” here matters, the numbers are too low, reducing them is not great, so lets not do that :).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
But this would unfortunately be a security issue for (some) L2s relying on it..
Then those L2s are broken. Policy is never a guarantee and should not be treated like one.
IMO you should reopen this.