Skip to content

Commit

Permalink
Fix: class CScript - dust rule should not apply to unspendable txouts
Browse files Browse the repository at this point in the history 
-OP_RETURN value of 0 is acceptable by protocol rules
  • Loading branch information
@zathras-crypto
zathras-crypto committed Mar 14, 2015
1 parent 41739bd commit 802ea70
Showing 1 changed file with 4 additions and 2 deletions.
6 changes: 4 additions & 2 deletions src/core.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,11 @@ class CTxOut
// to spend something, then we consider it dust.
// A typical txout is 34 bytes big, and will
// need a CTxIn of at least 148 bytes to spend,
// so dust is a txout less than 546 satoshis
// so dust is a txout less than 546 satoshis
// with default nMinRelayTxFee.
return ((nValue*1000)/(3*((int)GetSerializeSize(SER_DISK,0)+148)) < nMinRelayTxFee);
if (!scriptPubKey.IsUnspendable()) { // dust applicable to spendable outputs only
return ((nValue*1000)/(3*((int)GetSerializeSize(SER_DISK,0)+148)) < nMinRelayTxFee);
} else { return false; }
}

friend bool operator==(const CTxOut& a, const CTxOut& b)
Expand Down

4 comments on commit 802ea70

@dexX7
Copy link

@dexX7 dexX7 commented on 802ea70 Mar 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also do you think this 802ea70 should be submitted upstream?

Imho it seems more reasonable to move that check closer to the wallet/transaction creation code in ClassB_send (or where OP_RETURN transactions are going to be created).

@zathras-crypto
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CreateTransaction is a function of the wallet, and as part of its validity checks it verifies each output passes the dust check when creating a transaction.

I thought the most accurate approach was to make the response to IsDust false, since an unspendable txout is not considered dust.

We could for example override CreateTransaction in wallet.cpp to not perform the dust check for OP_RETURN outputs, but that didn't seem to provide proper coverage, since transaction creation is not the only place where the question of "is this txout dust?" might apply.

@dexX7
Copy link

@dexX7 dexX7 commented on 802ea70 Mar 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, sorry, I see. Let me take a look at this, even though I saw that line, I'm quite surprised actually.

@dexX7
Copy link

@dexX7 dexX7 commented on 802ea70 Mar 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it. I did some OP_RETURN tests earlier, but I just saw those were not created with zero amounts.

This is the full blown check:
https://github.com/zathras-crypto/mastercore/blob/0.0.9.2-Z-ClassC/src/main.cpp#L570-L589

  1. So how using that in combination or as replacement of:
    https://github.com/zathras-crypto/mastercore/blob/0.0.9.2-Z-ClassC/src/wallet.cpp#L1265-L1269

...?

Might even make sense to remove all those checks from wallet.cpp and simply call IsStandard(wtxNew, strFailReason) at the end.

  1. Nevertheless, you still have a valid point, unrelated to the wallet code, but I'd change the if-else to:
bool IsDust(int64_t nMinRelayTxFee) const
{
    if (scriptPubKey.IsUnspendable())
    { return false; }

    return ((nValue*1000)/(3*((int)GetSerializeSize(SER_DISK,0)+148)) < nMinRelayTxFee);
}
  1. Given that we also have a GetDustLimit(), which could be tweaked as well, we might jump ahead and adopt some (slightly adjusted) pending-to-be-merged-upstream-code and get rid of GetDustLimit():

https://github.com/bitcoin/bitcoin/pull/5831/files#diff-5cb8d9decaa15620a8f98b0c6c44da9bL137

Edit: some edits. ;)

Please sign in to comment.