From e2f99f51332ef9bf52aff42b2932d812ea7583b2 Mon Sep 17 00:00:00 2001 From: Daira Emma Hopwood Date: Wed, 12 Apr 2023 16:58:24 +0100 Subject: [PATCH] Fix the dust threshold rate to three times 100 zats/1000 bytes. (We express it that way rather than 300 zats/1000 bytes, because the threshold is always rounded to an integer and then multiplied by 3.) Bitcoin Core added the concept of "dust" in bitcoin/bitcoin#2577. At that point the dust threshold was tied to three times the minRelayTxFee rate, with the motivation that if you'd pay more than a third of the minimum relay fee to spend something, it should be considered dust. This was implemented as a standard rule rejecting dust outputs. This motivation will not apply after ZIP 317 block construction is implemented: at that point the ZIP 317 marginal fee will be 5000 zats per logical action, but the dust threshold rate will still be three times 100 zats per 1000 bytes. Those costs would only coincide if the marginal size per logical action were 5000/300 * 1000 ~= 16667 bytes, and in practice the marginal size for any kind of input is much smaller than that. However, to avoid interoperability problems (older wallets creating transactions that newer nodes will reject because they view the outputs as dust), we will have to coordinate any increase in the dust threshold carefully. More history: in Zcash the minRelayTxFee rate was 5000 zats/1000 bytes at launch, changed to 1000 zats/1000 bytes in zcashd v1.0.3 and to 100 zats/1000 bytes in zcashd v1.0.7-1 (#2141). The relaying problem for shielded transactions (#1969) that prompted the latter change was fixed more thoroughly by the addition of `CFeeRate::GetFeeForRelay` in #4916, ensuring that a transaction paying `DEFAULT_FEE` can always be relayed. At the same time the default fee was set to 1000 zats, per ZIP 313. An earlier commit in this PR changed relaying policy to be more strict about enforcing minRelayTxFee. The commit just before this one also allowed `-minrelaytxfee=0`, which we are going to use to avoid some test breakage. But if the dust threshold rate were still set to three times the minRelayTxFee rate, then setting `-minrelaytxfee=0` would have the side effect of setting the dust threshold to zero, which is not intended. Bitcoin Core took a different approach to disentangling the dust threshold from the relay threshold, adding a `-dustrelayfee` option (bitcoin/bitcoin#9380). We don't want to do that because it is likely that we will change the dust policy again, and adding a user-visible config option might conflict with that. Also, it isn't a good idea for the dust threshold rate to be configurable per node; it's a standard rule parameter and should only be changed with network-wide coordination (if it is increased then wallets have to change before nodes, and vice versa if it is decreased). So for now we set it to a constant that matches the behaviour before this PR. Since we can no longer modify the dust threshold, we remove a check from transaction_tests.cpp that relied on doing so. This change also indirectly fixes a false-positive assertion error that would occur in `SpendableInputs::LimitToAmount` if we allowed the dust threshold to be zero. Signed-off-by: Daira Emma Hopwood --- src/policy/policy.cpp | 2 +- src/policy/policy.h | 48 ++++++++++++++++++++++++++++++++ src/primitives/transaction.cpp | 17 +++++++++++ src/primitives/transaction.h | 21 ++------------ src/test/transaction_tests.cpp | 16 ++--------- src/wallet/wallet.cpp | 11 ++++---- src/wallet/wallet_tx_builder.cpp | 2 +- 7 files changed, 79 insertions(+), 38 deletions(-) diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index 967801e78..e44f9ac23 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -119,7 +119,7 @@ bool IsStandardTx(const CTransaction& tx, std::string& reason, const CChainParam else if ((whichType == TX_MULTISIG) && (!fIsBareMultisigStd)) { reason = "bare-multisig"; return false; - } else if (txout.IsDust(::minRelayTxFee)) { + } else if (txout.IsDust()) { reason = "dust"; return false; } diff --git a/src/policy/policy.h b/src/policy/policy.h index cd84e35f2..a2feb15b1 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -41,6 +41,54 @@ static const unsigned int STANDARD_SCRIPT_VERIFY_FLAGS = MANDATORY_SCRIPT_VERIFY /** For convenience, standard but not mandatory verify flags. */ static const unsigned int STANDARD_NOT_MANDATORY_VERIFY_FLAGS = STANDARD_SCRIPT_VERIFY_FLAGS & ~MANDATORY_SCRIPT_VERIFY_FLAGS; +/** + * The rate used to calculate the dust threshold, in zatoshis per 1000 bytes. + * This will effectively be multiplied by 3 (it is necessary to express it this + * way to ensure that rounding will be calculated the same as before #6542). + * + * Bitcoin Core added the concept of "dust" in bitcoin/bitcoin#2577. At that + * point the dust threshold rate was tied to three times the minRelayTxFee rate, + * with the motivation that if you'd pay more than a third of the minimum relay + * fee to spend something, it should be considered dust. This was implemented + * as a standard rule rejecting dust outputs. + * + * This motivation will not apply after ZIP 317 block construction is implemented: + * at that point the ZIP 317 marginal fee will be 5000 zats per logical action, + * but the dust threshold rate will still be 300 zats per 1000 bytes. Those costs + * would only coincide if the marginal size per logical action were + * 5000/300 * 1000 ~= 16667 bytes, and in practice the marginal size for any + * kind of input is much smaller than that. + * + * However, to avoid interoperability problems (older wallets creating transactions + * that newer nodes will reject because they view the outputs as dust), we will have + * to coordinate any increase in the dust threshold carefully. + * + * More history: in Zcash the minRelayTxFee rate was 5000 zats/1000 bytes at launch, + * changed to 1000 zats/1000 bytes in zcashd v1.0.3 and to 100 zats/1000 bytes in + * zcashd v1.0.7-1 (#2141). The relaying problem for shielded transactions (#1969) + * that prompted the latter change was fixed more thoroughly by the addition of + * `CFeeRate::GetFeeForRelay` in #4916, ensuring that a transaction paying + * `DEFAULT_FEE` can always be relayed. At the same time the default fee was set + * to 1000 zats, per ZIP 313. + * + * #6542 changed relaying policy to be more strict about enforcing minRelayTxFee. + * It also allowed `-minrelaytxfee=0`, which we are using to avoid some test + * breakage. But if the dust threshold rate were still set to three times the + * minRelayTxFee rate, then setting `-minrelaytxfee=0` would have the side effect + * of setting the dust threshold to zero, which is not intended. + * + * Bitcoin Core took a different approach to disentangling the dust threshold from + * the relay threshold, adding a `-dustrelayfee` option (bitcoin/bitcoin#9380). + * We don't want to do that because it is likely that we will change the dust policy + * again, and adding a user-visible config option might conflict with that. Also, + * it isn't a good idea for the dust threshold rate to be configurable per node; + * it's a standard rule parameter and should only be changed with network-wide + * coordination (if it is increased then wallets have to change before nodes, and + * vice versa if it is decreased). So for now we set it to a constant that matches + * the behaviour before #6542. + */ +static const unsigned int ONE_THIRD_DUST_THRESHOLD_RATE = 100; + // Sanity check the magic numbers when we change them static_assert(DEFAULT_BLOCK_MAX_SIZE <= MAX_BLOCK_SIZE); diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index c748f128a..2b3b0a835 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -5,6 +5,7 @@ // file COPYING or https://www.opensource.org/licenses/mit-license.php . #include "primitives/transaction.h" +#include "policy/policy.h" #include "hash.h" #include "tinyformat.h" @@ -123,6 +124,22 @@ CTxOut::CTxOut(const CAmount& nValueIn, CScript scriptPubKeyIn) scriptPubKey = scriptPubKeyIn; } +CAmount CTxOut::GetDustThreshold() const +{ + // See the comment on ONE_THIRD_DUST_THRESHOLD_RATE in policy.h. + static const CFeeRate oneThirdDustThresholdRate {ONE_THIRD_DUST_THRESHOLD_RATE}; + + if (scriptPubKey.IsUnspendable()) + return 0; + + // A typical spendable txout is 34 bytes, and will need a txin of at + // least 148 bytes to spend. With ONE_THIRD_DUST_THRESHOLD_RATE == 100, + // the dust threshold for such a txout would be + // 3*floor(100*(34 + 148)/1000) zats = 54 zats. + size_t nSize = GetSerializeSize(*this, SER_DISK, 0) + 148u; + return 3*oneThirdDustThresholdRate.GetFee(nSize); +} + uint256 CTxOut::GetHash() const { return SerializeHash(*this); diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index f569f8039..0b9b01ffb 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -625,26 +625,11 @@ public: uint256 GetHash() const; - CAmount GetDustThreshold(const CFeeRate &minRelayTxFee) const - { - // "Dust" is defined in terms of CTransaction::minRelayTxFee, - // which has units satoshis-per-kilobyte. - // If you'd pay more than 1/3 in fees - // to spend something, then we consider it dust. - // A typical spendable txout is 34 bytes big, and will - // need a CTxIn of at least 148 bytes to spend: - // so dust is a spendable txout less than - // 54*minRelayTxFee/1000 (in satoshis) - if (scriptPubKey.IsUnspendable()) - return 0; + CAmount GetDustThreshold() const; - size_t nSize = GetSerializeSize(*this, SER_DISK, 0) + 148u; - return 3*minRelayTxFee.GetFee(nSize); - } - - bool IsDust(const CFeeRate &minRelayTxFee) const + bool IsDust() const { - return (nValue < GetDustThreshold(minRelayTxFee)); + return nValue < GetDustThreshold(); } friend bool operator==(const CTxOut& a, const CTxOut& b) diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index e1c468984..ea9ac0caf 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -690,8 +690,9 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) string reason; BOOST_CHECK(IsStandardTx(t, reason, chainparams)); - // Check dust with default relay fee: - CAmount nDustThreshold = 182 * minRelayTxFee.GetFeePerK()/1000 * 3; + // Check dust threshold: + CFeeRate oneThirdDustThresholdRate{ONE_THIRD_DUST_THRESHOLD_RATE}; + CAmount nDustThreshold = (34 + 148) * oneThirdDustThresholdRate.GetFeePerK()/1000 * 3; BOOST_CHECK_EQUAL(nDustThreshold, 54); // dust: t.vout[0].nValue = nDustThreshold - 1; @@ -700,17 +701,6 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) t.vout[0].nValue = nDustThreshold; BOOST_CHECK(IsStandardTx(t, reason, chainparams)); - // Check dust with odd relay fee to verify rounding: - // nDustThreshold = 182 * 1234 / 1000 * 3 - minRelayTxFee = CFeeRate(1234); - // dust: - t.vout[0].nValue = 672 - 1; - BOOST_CHECK(!IsStandardTx(t, reason, chainparams)); - // not dust: - t.vout[0].nValue = 672; - BOOST_CHECK(IsStandardTx(t, reason, chainparams)); - minRelayTxFee = CFeeRate(DEFAULT_MIN_RELAY_TX_FEE); - t.vout[0].scriptPubKey = CScript() << OP_1; BOOST_CHECK(!IsStandardTx(t, reason, chainparams)); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a149394c1..629c54f7c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5558,7 +5558,7 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt } } - if (txout.IsDust(::minRelayTxFee)) + if (txout.IsDust()) { if (recipient.fSubtractFeeFromAmount && nFeeRet > 0) { @@ -5630,16 +5630,16 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt // We do not move dust-change to fees, because the sender would end up paying more than requested. // This would be against the purpose of the all-inclusive feature. // So instead we raise the change and deduct from the recipient. - if (nSubtractFeeFromAmount > 0 && newTxOut.IsDust(::minRelayTxFee)) + if (nSubtractFeeFromAmount > 0 && newTxOut.IsDust()) { - CAmount nDust = newTxOut.GetDustThreshold(::minRelayTxFee) - newTxOut.nValue; + CAmount nDust = newTxOut.GetDustThreshold() - newTxOut.nValue; newTxOut.nValue += nDust; // raise change until no more dust for (unsigned int i = 0; i < vecSend.size(); i++) // subtract from first recipient { if (vecSend[i].fSubtractFeeFromAmount) { txNew.vout[i].nValue -= nDust; - if (txNew.vout[i].IsDust(::minRelayTxFee)) + if (txNew.vout[i].IsDust()) { strFailReason = _("The transaction amount is too small to send after the fee has been deducted"); return false; @@ -5651,7 +5651,7 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt // Never create dust outputs; if we would, just // add the dust to the fee. - if (newTxOut.IsDust(::minRelayTxFee)) + if (newTxOut.IsDust()) { nFeeRet += nChange; reservekey.ReturnKey(); @@ -7753,6 +7753,7 @@ bool SpendableInputs::LimitToAmount( const CAmount dustThreshold, const std::set& recipientPools) { + // dustThreshold cannot be zero because it is no longer configured via `-minrelaytxfee`. assert(amountRequired >= 0 && dustThreshold > 0); // Calling this method twice is a programming error. assert(!limited); diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index 1576d1ba2..6bead7085 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -191,7 +191,7 @@ CAmount WalletTxBuilder::DefaultDustThreshold() const { CKey secret{CKey::TestOnlyRandomKey(true)}; CScript scriptPubKey = GetScriptForDestination(secret.GetPubKey().GetID()); CTxOut txout(CAmount(1), scriptPubKey); - return txout.GetDustThreshold(minRelayFee); + return txout.GetDustThreshold(); } SpendableInputs WalletTxBuilder::FindAllSpendableInputs(