[tests] Be more strict checking dust #6822

MarcoFalke commented at 6:00 PM on October 13, 2015: member

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

paveljanik commented at 6:13 PM on October 13, 2015: contributor

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 :-)

MarcoFalke commented at 6:21 PM on October 13, 2015: member

@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 commented at 6:35 PM on October 13, 2015: contributor

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

MarcoFalke force-pushed on Oct 13, 2015

MarcoFalke renamed this:
~~[trivial] minrelaytxfee cleanup~~
[tests] Be more strict checking dust
on Oct 13, 2015

MarcoFalke commented at 7:41 PM on October 13, 2015: member

Addressed comment NACK by @paveljanik

paveljanik commented at 7:50 PM on October 13, 2015: contributor

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

MarcoFalke force-pushed on Oct 13, 2015

sipa commented at 12:55 AM on October 14, 2015: member

utACK

TheBlueMatt commented at 10:05 AM on October 14, 2015: member

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 commented at 10:11 AM on October 14, 2015: member

@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.

MarcoFalke commented at 10:11 AM on October 14, 2015: member

@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 commented at 10:14 AM on October 14, 2015: member

@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.

MarcoFalke commented at 10:15 AM on October 14, 2015: member

@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 commented at 10:18 AM on October 14, 2015: member

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

TheBlueMatt commented at 10:19 AM on October 14, 2015: member

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 notifications@github.com 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 commented at 10:26 AM on October 14, 2015: contributor

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

MarcoFalke force-pushed on Oct 14, 2015

in src/primitives/transaction.h:None in 0cae2f583b outdated

 142 | @@ -143,8 +143,8 @@ class CTxOut
 143 |          // to spend something, then we consider it dust.
 144 |          // A typical spendable txout is 34 bytes big, and will
 145 |          // need a CTxIn of at least 148 bytes to spend:
 146 | -        // so dust is a spendable txout less than 546 satoshis
 147 | -        // with default minRelayTxFee.
 148 | +        // so dust is a spendable txout less than
 149 | +        // 546*minRelayTxFee/1000 (in satoshis)

dexX7 commented at 10:45 AM on October 14, 2015:

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

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

MarcoFalke commented at 10:49 AM on October 14, 2015: member

Addressed nit by @paveljanik and force pushed.

laanwj added the label Tests on Oct 26, 2015

laanwj commented at 9:23 AM on October 26, 2015: member

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 commented at 11:30 AM on October 26, 2015: contributor

(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.

MarcoFalke force-pushed on Oct 27, 2015

MarcoFalke commented at 10:14 AM on October 28, 2015: member

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

in src/test/transaction_tests.cpp:None in 3f4b5ac394 outdated

 341 | @@ -342,11 +342,26 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
 342 |      string reason;
 343 |      BOOST_CHECK(IsStandardTx(t, reason));
 344 |  
 345 | -    t.vout[0].nValue = 501; // dust
 346 | +    // Check dust with default relay fee:
 347 | +    CAmount nDustThreshold = 182 * minRelayTxFee.GetFeePerK()/1000 * 3 ;

dexX7 commented at 10:56 AM on October 28, 2015:

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.

MarcoFalke commented at 11:12 AM on October 28, 2015:

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

MarcoFalke commented at 11:35 AM on October 28, 2015:

Travis passes. Looks merge ready.

dexX7 commented at 3:06 PM on October 28, 2015:

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! :)

MarcoFalke force-pushed on Oct 28, 2015

MarcoFalke force-pushed on Nov 5, 2015

MarcoFalke commented at 8:15 PM on November 5, 2015: member

Rebased. Anything holding this back?

in src/main.h:None in 2f9f050470 outdated

  40 | @@ -41,6 +41,8 @@ struct CNodeStateStats;
  41 |  
  42 |  /** Default for accepting alerts from the P2P network. */
  43 |  static const bool DEFAULT_ALERTS = true;
  44 | +/** Default for -minrelaytxfee, minimum relay fee for transactions */
  45 | +static const unsigned int DEFAULT_MIN_RELAY_TX_FEE = 1000;

laanwj commented at 10:36 PM on November 5, 2015:

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

MarcoFalke commented at 7:59 AM on November 6, 2015:

@laanwj :ballot_box_with_check: done.

transaction_tests: Be more strict checking dust

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

5f46a7d068

[trivial] New DEFAULT_MIN_RELAY_TX_FEE = 1000 536766c903

[trivial] init: Use defaults MIN_RELAY_TX_FEE & TRANSACTION_MAXFEE e20d9245e5

MarcoFalke force-pushed on Nov 9, 2015

laanwj merged this on Nov 10, 2015

laanwj closed this on Nov 10, 2015

laanwj referenced this in commit 9fa54a1b0c on Nov 10, 2015

MarcoFalke deleted the branch on Nov 10, 2015

zkbot referenced this in commit 8713d73daf on Dec 18, 2019

zkbot referenced this in commit 2da77edbfe on Dec 18, 2019

zkbot referenced this in commit 577f7ef72a on Dec 18, 2019

DrahtBot locked this on Sep 8, 2021