Fees: Tests: Check CFeeRate internal precision in mempool_tests.cpp #7728

Closed
wants to merge 3 commits into
from

Projects

None yet

6 participants

@jtimon
Contributor
jtimon commented Mar 21, 2016

Although the risks are probably minimal or unexistent, #7727 shouldn't have passed travis' tests.
This adds a check that would fail with something like #7727.
I'm not saying there's a real risk in such a change passing review, but one more test shouldn't hurt.

A couple of constants are added that could be potentially useful if we ever intend to increase CFeeRate's internal precision multiplier above 1000 (aka KB), which I have no idea if it's the case right now, but I imagine could be something we want at some point in the future.

@MarcoFalke MarcoFalke and 1 other commented on an outdated diff Mar 21, 2016
src/amount.cpp
@@ -12,14 +12,14 @@ const std::string CURRENCY_UNIT = "BTC";
CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nSize)
{
if (nSize > 0)
- nSatoshisPerK = nFeePaid*1000/nSize;
+ nSatoshisPerK = nFeePaid * PRECISION_MULTIPLIER / nSize;
@MarcoFalke
MarcoFalke Mar 21, 2016 Member

You can't call this nSatoshisPerK anymore. If you "break" something, change its name.

@jtimon
jtimon Mar 21, 2016 Contributor

Yeah, please let me follow up this with an "only for discussion" (#7731). The name nSatoshisPerK is definitely part of what I don't like about CFeeRate and related to the point I want to make in that PR.

Maybe I should have left the constant PRECISION_MULTIPLIER = KB for that branch, but given that changing that PRECISION_MULTIPLIER = KB+1 will make the new test (among others) fail, I didn't saw how this makes the situation any worse.

@MarcoFalke
MarcoFalke Mar 21, 2016 Member

PRECISION_MULTIPLIER only makes sense being a multiple of KB, imo.

@jtimon
jtimon Mar 21, 2016 Contributor

Yeah, see #7731 . GetFee is over complicated there, but if you assert assert(PRECISION_MULTIPLIER % KB == 0) that code can be simplified. I mean, it can be simplified by more means, for example, removing the special case from #7705 (comment) (although that will be greatly disruptive outside of the class).

@laanwj laanwj added the Tests label Mar 24, 2016
@jtimon
Contributor
jtimon commented Mar 29, 2016

Ping, should I remove constant PRECISION_MULTIPLIER (just use KB for now) as suggested by @MarcoFalke ?
Is there anything else that I should change before merging?

@MarcoFalke
Member

Yes, either remove PRECISION_MULTIPLIER or rename nSatoshisPerK to nSatoshis or nSatoshisRate.

@laanwj laanwj and 1 other commented on an outdated diff Mar 31, 2016
@@ -15,6 +15,8 @@ typedef int64_t CAmount;
static const CAmount COIN = 100000000;
static const CAmount CENT = 1000000;
+static const size_t KB = 1000;
+static const size_t PRECISION_MULTIPLIER = KB;
@laanwj
laanwj Mar 31, 2016 Member

This name is too general, people will confuse this with something innate to bitcoin (like COIN). FEERATE_PRECISION_MULTIPLIER maybe?
Edit: Or even better, make it a constant inside class CFeeRate?

@jtimon
jtimon Mar 31, 2016 Contributor

Yeah, FEERATE_PRECISION_MULTIPLIER is much more clear (I think I started with that before confusing myself with 2 _PRECISION_MULTIPLIER constants).
About making it a static const attribute in CFeeRate...I would like to avoid that.
Unless I'm missing something, such a constant would need to be initialized in the cpp. That is, for example, what I dislike about https://github.com/bitcoin/bitcoin/blob/master/src/chainparamsbase.cpp#L13

You cannot, for example, use those "constants" in https://github.com/bitcoin/bitcoin/blob/master/src/qt/networkstyle.cpp#L18 (it will fail because the string constants are not initialized!).

Anyway, minor details. The main reason I left the constant there is because of the comment in https://github.com/bitcoin/bitcoin/pull/7728/files#diff-1d763a8b6d8d21b8a184426c1524ba2aR575 and for "git history premature optimization?". I'm happy with both renaming it to FEERATE_PRECISION_MULTIPLIER or leaving it for something replacing #7731 (which I think I can close now, even if I'm still interested in other people's thoughts on it).

@laanwj
laanwj Mar 31, 2016 Member

Then we agree that it's confusing. Please do rename it for this pull.

Unless I'm missing something, such a constant would need to be initialized in the cpp. That is, for example, what I dislike about

Not true, try to compile the following:

#include <stdio.h>
class TestClass {
public:
    static const int SubValue = 12345;
};
int main()
{
    printf("%i\n", TestClass::SubValue);
}

String constants and other objects need to be initialized in the cpp, no matter where they're defined. But integral types (such as ints) can be initialized in the header.

@jtimon
jtimon Mar 31, 2016 Contributor

Oh, I see, this is not a problem for basic types (I always forget strings are not a basic type in C++). Then I'm happy with either FEERATE_PRECISION_MULTIPLIER or CFeeRate::PRECISION_MULTIPLIER.

Offtopic: back to the chainparams petname strings. Would it make sense to eventually (and probably very slowly to minimize disruption) replace those static strings with macros (so that "main" and friends can be replaced everywhere)?

@MarcoFalke
Member

Also you may add the tests to https://github.com/bitcoin/bitcoin/blob/28ad4d9fc2be102786a8c6c32ebecb466b2a03dd/src/test/amount_tests.cpp, so they are not "hidden" inside the mempool tests.

@jtimon
Contributor
jtimon commented Mar 31, 2016

@MarcoFalke Mhmm, you mean something like:

BOOST_CHECK_EQUAL(feeRate.GetFee(X), 1557);
BOOST_CHECK_EQUAL(feeRate.GetFee(Y), 1000);
BOOST_CHECK_EQUAL(feeRate.GetFee(Z), 0);

or something more dynamic? Sounds like a good idea, in principle.
I can take a look, but if you have something very clear in mind, please, I'm happy to take another commit (unless it is preferred to leave it for another PR).

@MarcoFalke
Member

What I meant was to also test the CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nSize) constructor and then do two or three GetFee() on the instance.

@jtimon
Contributor
jtimon commented Mar 31, 2016

Updated, hopefully solving nits.
The latest tests added (last commit) are probably overkill, but if it's going to take a lot of back and forth in review, I would rather leave it for another PR.

@MarcoFalke MarcoFalke and 2 others commented on an outdated diff Mar 31, 2016
src/test/amount_tests.cpp
@@ -37,6 +37,31 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
BOOST_CHECK_EQUAL(feeRate.GetFee(999), 122);
BOOST_CHECK_EQUAL(feeRate.GetFee(1e3), 123);
BOOST_CHECK_EQUAL(feeRate.GetFee(9e3), 1107);
+
+ // With full constructor (allows to test internal precission)
+ feeRate = CFeeRate(KB, KB * 2);
@MarcoFalke
MarcoFalke Mar 31, 2016 Member

Lets not confuse people by saying I paid a fee of (KB) satoshis.

@jtimon
jtimon Mar 31, 2016 Contributor

Please, improve and bike-shed the commit at will and I will probably squash. I warned about this, didn't I?

@laanwj
laanwj Apr 3, 2016 Member

Tend to agree here. Only use the KB constant for sizes, not for amounts. If you want a constant for 1000 satoshis, that's fine, define a differently-named one (only for the test, probably).

@MarcoFalke MarcoFalke and 1 other commented on an outdated diff Apr 2, 2016
src/amount.h
@@ -36,8 +37,10 @@ inline bool MoneyRange(const CAmount& nValue) { return (nValue >= 0 && nValue <=
class CFeeRate
{
private:
- CAmount nSatoshisPerK; // unit is satoshis-per-1,000-bytes
+ CAmount nSatoshisPerK; //! unit is satoshis-per-PRECISION_MULTIPLIER-bytes
@MarcoFalke
MarcoFalke Apr 2, 2016 Member

I still think this commit is not ready to merge. You can not introduce a new internal precision multiplier (which happens to default to 1k) and then not adjust the name of the variable it affects.

Somone will change the prec. multiplier to MB one day and no one will notice that the meaning of nSatoshisPerK changed.

@jtimon
jtimon Apr 2, 2016 Contributor

See #7731. You cannot *2 without disruption. *10 requires more disruption, etc. Besides, this includes new tests that would fail when changed. Nobody will change the constant "without noticing".

@MarcoFalke
MarcoFalke Apr 2, 2016 Member

Fine, then just remove PRECISION_MULTIPLIER.

@jtimon
Contributor
jtimon commented Apr 2, 2016

I'm not renaming nSatoshisPerK in this PR. I said that I was happy with either renaming or removing the constant and I thought renaming was enough. Please @MarcoFalke @laanwj can you decide one or the other? I don't care enough to discuss about it, but I cannot have the constant and not have it at the same time.

@laanwj
Member
laanwj commented Apr 3, 2016

I'm not renaming nSatoshisPerK in this PR

I tend to agree that renaming the GetFeePerK method has a much larger diff impact, and should not be done here. Let's leave this to a pull that would actually change PRECISION_MULTIPLIER from 1000. I am sure there will be other work to do as well at call-sites that expect a number per K.

Same for renaming the member variable nSatoshisPerK, probably. Although it's only used in amount internally, so the above case is harder to make.

@jtimon
Contributor
jtimon commented Apr 3, 2016

So the simplest options are still to leave the constant a is or to also leave the constant for later (which is what @MarcoFalke prefers). My preference would be to leave it as it is, but I'm happy removing the constant for now as well.

@laanwj
Member
laanwj commented Apr 3, 2016

I'm fine with leaving the constant.
I'd say add an assert(PRECISION_MULTIPLIER==1000); to GetFeePerK() make this concern concrete.

@jtimon
Contributor
jtimon commented Apr 6, 2016

I don't think the assert is necessary and I would prefer to just leave the the constant for later than adding it.

@MarcoFalke
Member

Agree that the assert is overkill. Maybe just add a comment, but it seems there is too much discussion about this; Please fix the amount tests, so we can move forward with this.

@jtimon
Contributor
jtimon commented Apr 14, 2016

Rebased (1) after #7796.
Also added the comment on PRECISION_MULTIPLIER and stopped using KB for amounts in amount_tests.cpp.

Surprisingly enough, #7796 makes the introduction of PRECISION_MULTIPLIER fail. I separated the failing change on the last commit labelled with ERROR ( 5ca2473 ) but I really don't understand what is happening here. In the same way, the fisrt commit fails without the fixup after it.

@MarcoFalke MarcoFalke and 1 other commented on an outdated diff Apr 14, 2016
src/amount.cpp
@@ -15,7 +15,7 @@ CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nBytes_)
int64_t nSize = int64_t(nBytes_);
if (nSize > 0)
- nSatoshisPerK = nFeePaid * 1000 / nSize;
+ nSatoshisPerK = nFeePaid * PRECISION_MULTIPLIER / nSize;
@MarcoFalke
MarcoFalke Apr 14, 2016 Member

This won't work. PRECISION_MULTIPLIER is unsigned (either 32-bit or 64-bit) as of your current implementation and cpp will convert all other integers to unsigned 64-bit in this line on 64-bit platforms.

@jtimon
jtimon Apr 14, 2016 Contributor

Yes, it seems that was it, 4cb815a seems to solve it (although it feels wrong for PRECISION_MULTIPLIER not to be size_t).

I will definitely remove it (that's what I should have done the first you complained, instead of moving it around to keep discussing about it), but since half of the PR discussion ended up being around that constant, I don't care about waiting a little bit longer to finish discussing it (in case is used in a future PR replacing #7731 uses it).

@jtimon
Contributor
jtimon commented Apr 14, 2016

Removed the PRECISION_MULTIPLIER=KB constant.

@MarcoFalke MarcoFalke commented on an outdated diff Apr 29, 2016
src/test/mempool_tests.cpp
@@ -566,11 +566,19 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
BOOST_CHECK_EQUAL(pool.GetMinFee(pool.DynamicMemoryUsage() * 9 / 2).GetFeePerK(), (maxFeeRateRemoved.GetFeePerK() + 1000)/8);
// ... with a 1/4 halflife when mempool is < 1/4 its target size
- SetMockTime(42 + 7*CTxMemPool::ROLLING_FEE_HALFLIFE + CTxMemPool::ROLLING_FEE_HALFLIFE/2 + CTxMemPool::ROLLING_FEE_HALFLIFE/4);
+ unsigned i = 5;
+ while (pool.GetMinFee(1).GetFeePerK() > (int)(2 * KB)) {
+ SetMockTime(42 + (++i)*CTxMemPool::ROLLING_FEE_HALFLIFE + CTxMemPool::ROLLING_FEE_HALFLIFE/2 + CTxMemPool::ROLLING_FEE_HALFLIFE/4);
+ }
+ // test increased stored precission in CFeeRate
@MarcoFalke
MarcoFalke Apr 29, 2016 Member

nit: typo

@MarcoFalke MarcoFalke and 1 other commented on an outdated diff Apr 29, 2016
src/test/mempool_tests.cpp
@@ -566,11 +566,19 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
BOOST_CHECK_EQUAL(pool.GetMinFee(pool.DynamicMemoryUsage() * 9 / 2).GetFeePerK(), (maxFeeRateRemoved.GetFeePerK() + 1000)/8);
// ... with a 1/4 halflife when mempool is < 1/4 its target size
- SetMockTime(42 + 7*CTxMemPool::ROLLING_FEE_HALFLIFE + CTxMemPool::ROLLING_FEE_HALFLIFE/2 + CTxMemPool::ROLLING_FEE_HALFLIFE/4);
+ unsigned i = 5;
+ while (pool.GetMinFee(1).GetFeePerK() > (int)(2 * KB)) {
@MarcoFalke
MarcoFalke Apr 29, 2016 edited Member

What is the meaning of 2KB here? You mean CAmount(2000)?

Edit: At least add a comment why the loop is necessary in the mempool tests to check the precision of CFeeRate...

@jtimon
jtimon Apr 29, 2016 Contributor

It wa clearer when it was CFeeRate::INTERNAL_PRECISION than now that it's KB...
I can add comments, suggested comments always preferred, otherwise I'll come up with something myself ...There's one comment about the internal prevision below, but maybe that's not clear enough.

@MarcoFalke
MarcoFalke May 2, 2016 Member

Hmm, what I mean is that you are modifying src/test/mempool_tests.cpp, which goal should be to test mempool logic. However, you are trying to add fee/feerate/amount tests, which seem orthogonal to mempool tests. Personally I find it harder to read the mempool test now. So if it is really required to modify the mempool test, you'd need to add a comment why it is being done here. Otherwise, I'd suggest to move the tests to src/test/amount_tests.cpp or similar.

@jtimon
jtimon May 20, 2016 Contributor

I added tests to amount_tests.cpp just because you asked for them.
If we ever increase CFeeRate internal precission, the changes in mempool_tests.cpp will help. With these changes, the mempool tests will also fail if #7727 (although with your latest additions to amount_tests, it #7727 should already fail).
See outdated and closed #7731 .

@sipa
Member
sipa commented Jun 3, 2016

Concept ACK (the test test exact precision, which is something that can change without problems, but the tests are marked as such).

@jtimon
Contributor
jtimon commented Aug 30, 2016

ping

@MarcoFalke
Member

Sorry for letting this sit around for so long. Though, I'd prefer to have at least an utACK on the mempool_tests.cpp changes, as I am still not convinced that the changes make the tests easier to read nor that this is the right place to test this logic.

Preferably one of the authors of mempool_tests.cpp can jump in to provide feedback.

@luke-jr
Member
luke-jr commented Sep 10, 2016

utACK everything except mempool_tests (but no objection to it either; sorry, I didn't understand what was going on there)

@MarcoFalke
Member

I didn't understand what was going on there

In which case we probably shouldn't interwind mempool checks with internal precision checks of CFeeRate.

@jtimon
Contributor
jtimon commented Sep 11, 2016

To remind people, the initial PR contained ONLY the changes in mempool_tests. When/if we change internal precision, we will have to make this kind of changes to the mempool tests. Those tests are inherently intertwined with the feerate internal precission. We can wait for when/if we change the precision. Then marcoFalke asked for adding tests to amount_tests, which I did. Since then more tests have been merged into amount_tests, so maybe my additional tests are now redundant?
I can remove parts or just close the PR, I was just pinging because it's been a while and I would like to get this resolved one way or another.

@morcos
Contributor
morcos commented Sep 12, 2016

I'm not sure we're ever going to change feerate internal precision. But yes, lets leave it until we do to change the mempool_tests. They are already annoyingly complicated enough as is (sorry), no reason to make them more convoluted; the next change should make them easier to understand.

I don't object to the other changes, but I'm not sure they're worth bothering with.

@jtimon
Contributor
jtimon commented Sep 12, 2016

I don't object to the other changes, but I'm not sure they're worth bothering with.

I tend to agree, as said new tests were added after I added those tests and now these seem kind of redundant (ie #7727 would now fail the tests as it should).

Since people don't seem to like the changes on mempool, I'm leaning towards closing.

@MarcoFalke
Member

Closing for now per above discussion.

@MarcoFalke MarcoFalke closed this Sep 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment