[tests] Be more strict checking dust #6822

maflcko · 2015-10-13T18:00:39Z

The dust test case is too fuzzy. This PR makes transaction_tests more strict when checking dust.

paveljanik · 2015-10-13T18:13:29Z

init.h: not worth to change IMO

transaction.h: NACK, the comment on spendable output is important there.

transactions_tests: ACK - I like such changes :-)

maflcko · 2015-10-13T18:21:34Z

@paveljanik transaction.h: NACK, the comment on spendable output is important there.

Spendable is mentioned in L144 as well but I am happy to revert that if others agree. (Personally, I don't like magic numbers in comments)

paveljanik · 2015-10-13T18:35:19Z

Don't get me wrong: the comment has to be changed. Maybe change the magic numbers to some math formula there? Something like:

// So dust is a spendable txout less than 3*(34+148)*(minRelayTxFee/1000) satoshis

maflcko · 2015-10-13T19:41:46Z

Addressed comment NACK by @paveljanik

paveljanik · 2015-10-13T19:50:37Z

Can you also change 2730 (ie. magic number :-) into the above mentioned math. formula?

sipa · 2015-10-14T00:55:14Z

utACK

TheBlueMatt · 2015-10-14T10:05:27Z

Is it just me or does this gratuitously conflict with another outstanding PR? When proposing a PR that isnt designed to replace another, but will make it fail CI (or otherwise not merge cleanly), can you either rebase your work on the other, or, at a minimum, mention it on the PR that it effects?

sipa · 2015-10-14T10:11:06Z

@TheBlueMatt Please... it's hard enough for reviewers to keep up with all proposed patches, you can't expect all contributors to do so as well. It would be helpful if you mentioned what PR it is, and suggest how it could be adapted to keep working.

maflcko · 2015-10-14T10:11:50Z

@paveljanik 8d02c78 is for you but imo the tests should not replace or duplicate the documentation of the source code.

@TheBlueMatt No worries, I have this already in mind and request a rebase on your PR once this is merged.

TheBlueMatt · 2015-10-14T10:14:03Z

@sipa I only bring it up because it was mentioned in the opening commit, only to not be tagged or worked around. I almost missed this.

maflcko · 2015-10-14T10:15:52Z

@TheBlueMatt If you want me to change the default in this PR, making yours a little bit cleaner, I am happy to do so. (Given that dropping the minRelayTxFee for the master branch is uncontroversial)

sipa · 2015-10-14T10:18:53Z

This PR could define the dust limit variable in function of minTxRelayFee?

TheBlueMatt · 2015-10-14T10:19:22Z

Naa, I can do it in #6722 if this is merged first... I just wanted to make sure it was tagged so that it's visible. You could include the commit that changed the min relay fee if you prefer, but if you want to do that probably wait until mempool limiting is merged anyway.

On October 14, 2015 3:16:02 AM PDT, MarcoFalke [email protected] wrote:

@TheBlueMatt If you want me to change the default in this PR, making
yours a little bit cleaner, I am happy to do so. (Given that dropping
the minRelayTxFee for the master branch is uncontroversial)

Reply to this email directly or view it on GitHub:
#6822 (comment)

paveljanik · 2015-10-14T10:26:39Z

@MarcoFalke I just wanted to prevent this issue (forgotten update of this test) in the future. The suggested change doesn't prevent it though.

dexX7 · 2015-10-14T10:45:12Z

src/primitives/transaction.h

Nit: 546 is only accurate for pay-to-pubkey-hash outputs.

Edit: nevermind.. I guess that's covered by "a typical spendable txout ...".

maflcko · 2015-10-14T10:49:14Z

Addressed nit by @paveljanik and force pushed.

laanwj · 2015-10-26T09:23:00Z

I just wanted to prevent this issue (forgotten update of this test) in the future. The suggested change doesn't prevent it though.

On the other hand having to update the test with fixed values was a good reminder that changing minTxFee also changes the dust threshold. At least I hadn't realized this at first.
(I think adding a test with computed, exact threshold value is a good idea, but not entirely convinced it should replace the fixed values)

dexX7 · 2015-10-26T11:30:55Z

(I think adding a test with computed, exact threshold value is a good idea, but not entirely convinced it should replace the fixed values)

There are potentially a few different things to test:

the global minRelayTxFee is 1000 (or 5000)
the dust threshold calculation is 3*aFeeRate.GetFee(nSize)
the dust threshold calculation in IsStandardTx() is based on the minRelayTxFee

Using magic numbers to ensure the calculation with reference values results in expected values can be useful to pin down issues with the calculation, and the alternative could be to temporarily overwrite the global minRelayTxFee, and then set it back after the tests, e.g.:

CFeeRate minRelayTxFeeOriginal = minRelayTxFee;
minRelayTxFee = CFeeRate(1234);
// dust:
t.vout[0].nValue = 672 - 1;
BOOST_CHECK(!IsStandardTx(t, reason));
// not dust:
t.vout[0].nValue = 672;
BOOST_CHECK(IsStandardTx(t, reason));
minRelayTxFee = minRelayTxFeeOriginal;

In fact, and as a side note, with 1234 as minRelayTxFee, the dust threshold changed from 0.9 to 0.10 by two satoshi (from 674 to 672) due to the introduction of CFeeRate and rounding effects.

maflcko · 2015-10-28T10:14:45Z

@dexX7 Let me know if the nits are fixed so I can squash the commit.

dexX7 · 2015-10-28T10:56:22Z

src/test/transaction_tests.cpp

There is an unneeded space at the end.

Other than that, for this line you may consider using:

CAmount nDustThreshold = t.vout[0].GetDustThreshold(minRelayTxFee);

It should have a similar effect. Since hardcoded values are used (such as 546), it might be good to specify the "default relay fee" in the comment one line above.

GetDustThreshold is called via IsStandardTx so for the tests it's better not used here, imo. But thanks for the white space nit.

Travis passes. Looks merge ready.

GetDustThreshold is called via IsStandardTx so for the tests it's better not used here, imo.

Yeah, I guess it doesn't make much difference.

But thanks for the white space nit.

You're welcome! :)

maflcko · 2015-11-05T20:15:38Z

Rebased.
Anything holding this back?

laanwj · 2015-11-05T22:36:55Z

src/main.h

As you define this constant, please also use it in init.cpp, for example when printing the option help or when parsing the option.

@laanwj ☑️ done.

* Don't allow off-by-one or more * Make clear dust is coupled with minRelayTxFee * Check rounding for odd values

e20d924 [trivial] init: Use defaults MIN_RELAY_TX_FEE & TRANSACTION_MAXFEE (MarcoFalke) 536766c [trivial] New DEFAULT_MIN_RELAY_TX_FEE = 1000 (MarcoFalke) 5f46a7d transaction_tests: Be more strict checking dust (MarcoFalke)

Bitcoin 0.12 cleanup PRs 2 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6631 - bitcoin/bitcoin#6664 - Only the first commit (we already had the second through bitcoin/bitcoin#6825). - bitcoin/bitcoin#6669 - bitcoin/bitcoin#6887 - Only the non-QT parts. - bitcoin/bitcoin#6962 - bitcoin/bitcoin#6822 - Only first and third commits (we already had the second through an earlier PR). - bitcoin/bitcoin#7136 - Excludes Travis CI changes, and fixes to documents we don't have anymore. - bitcoin/bitcoin#7084 - bitcoin/bitcoin#7509 - bitcoin/bitcoin#7617 - bitcoin/bitcoin#7726 Part of #2074.

maflcko force-pushed the MarcoFalke-2015-minRelayTxFeeCleanup branch 2 times, most recently from 54d6df9 to 6351b1e Compare October 13, 2015 19:37

maflcko changed the title ~~[trivial] minrelaytxfee cleanup~~ [tests] Be more strict checking dust Oct 13, 2015

maflcko force-pushed the MarcoFalke-2015-minRelayTxFeeCleanup branch 3 times, most recently from f05296f to 03a7a44 Compare October 13, 2015 20:11

maflcko force-pushed the MarcoFalke-2015-minRelayTxFeeCleanup branch from 8d02c78 to 0cae2f5 Compare October 14, 2015 10:42

dexX7 reviewed Oct 14, 2015
View reviewed changes

laanwj added the Tests label Oct 26, 2015

maflcko force-pushed the MarcoFalke-2015-minRelayTxFeeCleanup branch from 0cae2f5 to 3f4b5ac Compare October 27, 2015 16:38

dexX7 reviewed Oct 28, 2015
View reviewed changes

maflcko force-pushed the MarcoFalke-2015-minRelayTxFeeCleanup branch from 3f4b5ac to 7ba5a4a Compare October 28, 2015 11:09

maflcko force-pushed the MarcoFalke-2015-minRelayTxFeeCleanup branch from 7ba5a4a to 2f9f050 Compare November 5, 2015 19:43

laanwj reviewed Nov 5, 2015
View reviewed changes

MarcoFalke added 3 commits November 9, 2015 21:00

transaction_tests: Be more strict checking dust

5f46a7d

* Don't allow off-by-one or more * Make clear dust is coupled with minRelayTxFee * Check rounding for odd values

[trivial] New DEFAULT_MIN_RELAY_TX_FEE = 1000

536766c

[trivial] init: Use defaults MIN_RELAY_TX_FEE & TRANSACTION_MAXFEE

e20d924

maflcko force-pushed the MarcoFalke-2015-minRelayTxFeeCleanup branch from f526958 to e20d924 Compare November 9, 2015 20:13

laanwj merged commit e20d924 into bitcoin:master Nov 10, 2015

maflcko deleted the MarcoFalke-2015-minRelayTxFeeCleanup branch November 10, 2015 16:29

str4d mentioned this pull request Dec 17, 2019

Bitcoin 0.12 cleanup PRs 2 zcash/zcash#4268

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[tests] Be more strict checking dust #6822

[tests] Be more strict checking dust #6822

Uh oh!

Conversation

maflcko commented Oct 13, 2015

Uh oh!

paveljanik commented Oct 13, 2015

Uh oh!

maflcko commented Oct 13, 2015

Uh oh!

paveljanik commented Oct 13, 2015

Uh oh!

maflcko commented Oct 13, 2015

Uh oh!

paveljanik commented Oct 13, 2015

Uh oh!

sipa commented Oct 14, 2015

Uh oh!

TheBlueMatt commented Oct 14, 2015

Uh oh!

sipa commented Oct 14, 2015 via email

Uh oh!

maflcko commented Oct 14, 2015

Uh oh!

TheBlueMatt commented Oct 14, 2015

Uh oh!

maflcko commented Oct 14, 2015

Uh oh!

sipa commented Oct 14, 2015 via email

Uh oh!

TheBlueMatt commented Oct 14, 2015

Uh oh!

paveljanik commented Oct 14, 2015

Uh oh!

dexX7 Oct 14, 2015

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 14, 2015

Uh oh!

laanwj commented Oct 26, 2015

Uh oh!

dexX7 commented Oct 26, 2015

Uh oh!

maflcko commented Oct 28, 2015

Uh oh!

dexX7 Oct 28, 2015

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 28, 2015

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 28, 2015

Choose a reason for hiding this comment

Uh oh!

dexX7 Oct 28, 2015

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 5, 2015

Uh oh!

laanwj Nov 5, 2015

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 6, 2015

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants