[tests] Be more strict checking dust #6822

Merged
merged 3 commits into from Nov 10, 2015

Conversation

Projects
None yet
6 participants
@MarcoFalke
Member

MarcoFalke commented Oct 13, 2015

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

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

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

Contributor

paveljanik commented Oct 13, 2015

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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 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)

Member

MarcoFalke commented Oct 13, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Oct 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
Contributor

paveljanik commented Oct 13, 2015

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 MarcoFalke changed the title from [trivial] minrelaytxfee cleanup to [tests] Be more strict checking dust Oct 13, 2015

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 13, 2015

Member

Addressed comment NACK by @paveljanik

Member

MarcoFalke commented Oct 13, 2015

Addressed comment NACK by @paveljanik

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Oct 13, 2015

Contributor

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

Contributor

paveljanik commented Oct 13, 2015

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 14, 2015

Member

utACK

Member

sipa commented Oct 14, 2015

utACK

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Oct 14, 2015

Contributor

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?

Contributor

TheBlueMatt commented Oct 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 14, 2015

Member
Member

sipa commented Oct 14, 2015

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 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.

Member

MarcoFalke commented Oct 14, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Oct 14, 2015

Contributor

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

Contributor

TheBlueMatt commented Oct 14, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 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)

Member

MarcoFalke commented Oct 14, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 14, 2015

Member
Member

sipa commented Oct 14, 2015

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Oct 14, 2015

Contributor

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)

Contributor

TheBlueMatt commented Oct 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Oct 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.

Contributor

paveljanik commented Oct 14, 2015

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

- // so dust is a spendable txout less than 546 satoshis
- // with default minRelayTxFee.
+ // so dust is a spendable txout less than
+ // 546*minRelayTxFee/1000 (in satoshis)

This comment has been minimized.

@dexX7

dexX7 Oct 14, 2015

Contributor

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

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

@dexX7

dexX7 Oct 14, 2015

Contributor

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

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

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 14, 2015

Member

Addressed nit by @paveljanik and force pushed.

Member

MarcoFalke commented Oct 14, 2015

Addressed nit by @paveljanik and force pushed.

@laanwj laanwj added the Tests label Oct 26, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 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)

Member

laanwj commented Oct 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Oct 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:

  1. the global minRelayTxFee is 1000 (or 5000)
  2. the dust threshold calculation is 3*aFeeRate.GetFee(nSize)
  3. 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.

Contributor

dexX7 commented Oct 26, 2015

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

  1. the global minRelayTxFee is 1000 (or 5000)
  2. the dust threshold calculation is 3*aFeeRate.GetFee(nSize)
  3. 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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 28, 2015

Member

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

Member

MarcoFalke commented Oct 28, 2015

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

@dexX7

View changes

src/test/transaction_tests.cpp
@@ -342,11 +342,26 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
string reason;
BOOST_CHECK(IsStandardTx(t, reason));
- t.vout[0].nValue = 501; // dust
+ // Check dust with default relay fee:
+ CAmount nDustThreshold = 182 * minRelayTxFee.GetFeePerK()/1000 * 3 ;

This comment has been minimized.

@dexX7

dexX7 Oct 28, 2015

Contributor

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.

@dexX7

dexX7 Oct 28, 2015

Contributor

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.

This comment has been minimized.

@MarcoFalke

MarcoFalke Oct 28, 2015

Member

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

@MarcoFalke

MarcoFalke Oct 28, 2015

Member

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Oct 28, 2015

Member

Travis passes. Looks merge ready.

@MarcoFalke

MarcoFalke Oct 28, 2015

Member

Travis passes. Looks merge ready.

This comment has been minimized.

@dexX7

dexX7 Oct 28, 2015

Contributor

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

@dexX7

dexX7 Oct 28, 2015

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 5, 2015

Member

Rebased.
Anything holding this back?

Member

MarcoFalke commented Nov 5, 2015

Rebased.
Anything holding this back?

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

This comment has been minimized.

@laanwj

laanwj Nov 5, 2015

Member

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

laanwj Nov 5, 2015

Member

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 6, 2015

Member

@laanwj ☑️ done.

@MarcoFalke

MarcoFalke Nov 6, 2015

Member

@laanwj ☑️ done.

MarcoFalke added some commits Oct 13, 2015

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 10, 2015

Merge pull request #6822
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)

@MarcoFalke MarcoFalke deleted the MarcoFalke:MarcoFalke-2015-minRelayTxFeeCleanup branch Nov 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment