Fees: Tests: Check CFeeRate internal precision in mempool

jtimon commented at 3:42 pm on March 21, 2016: contributor

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.

in src/amount.cpp: in 7a40286d62 outdated

11@@ -12,14 +12,14 @@ const std::string CURRENCY_UNIT = "BTC";
12 CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nSize)
13 {
14     if (nSize > 0)
15-        nSatoshisPerK = nFeePaid*1000/nSize;
16+        nSatoshisPerK = nFeePaid * PRECISION_MULTIPLIER / nSize;

MarcoFalke commented at 4:17 pm on March 21, 2016:

You can’t call this nSatoshisPerK anymore. If you “break” something, change its name.

jtimon commented at 5:18 pm on March 21, 2016:

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 commented at 5:21 pm on March 21, 2016:

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

jtimon commented at 7:59 pm on March 21, 2016:

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 (review) (although that will be greatly disruptive outside of the class).

jtimon force-pushed on Mar 21, 2016

laanwj added the label Tests on Mar 24, 2016

jtimon commented at 1:05 pm on March 29, 2016: contributor

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 commented at 5:53 pm on March 29, 2016: member

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

in src/amount.h: in 1fb9edd156 outdated

14@@ -15,6 +15,8 @@ typedef int64_t CAmount;
15 
16 static const CAmount COIN = 100000000;
17 static const CAmount CENT = 1000000;
18+static const size_t KB = 1000;
19+static const size_t PRECISION_MULTIPLIER = KB;

laanwj commented at 1:08 pm on March 31, 2016:

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 commented at 1:34 pm on March 31, 2016:

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 commented at 1:42 pm on March 31, 2016:

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:

0#include <stdio.h>
1class TestClass {
2public:
3    static const int SubValue = 12345;
4};
5int main()
6{
7    printf("%i\n", TestClass::SubValue);
8}

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 commented at 1:52 pm on March 31, 2016:

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 commented at 1:25 pm on March 31, 2016: 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 commented at 1:45 pm on March 31, 2016: contributor

@MarcoFalke Mhmm, you mean something like:

0BOOST_CHECK_EQUAL(feeRate.GetFee(X), 1557);
1BOOST_CHECK_EQUAL(feeRate.GetFee(Y), 1000);
2BOOST_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 commented at 1:50 pm on March 31, 2016: 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 force-pushed on Mar 31, 2016

jtimon commented at 2:49 pm on March 31, 2016: contributor

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.

in src/test/amount_tests.cpp: in cfd77f1cb3 outdated

36@@ -37,6 +37,31 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
37     BOOST_CHECK_EQUAL(feeRate.GetFee(999), 122);
38     BOOST_CHECK_EQUAL(feeRate.GetFee(1e3), 123);
39     BOOST_CHECK_EQUAL(feeRate.GetFee(9e3), 1107);
40+
41+    // With full constructor (allows to test internal precission)
42+    feeRate = CFeeRate(KB, KB * 2);

MarcoFalke commented at 3:15 pm on March 31, 2016:

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

jtimon commented at 11:52 pm on March 31, 2016:

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

laanwj commented at 1:20 pm on April 3, 2016:

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

in src/amount.h: in cfd77f1cb3 outdated

36@@ -36,8 +37,10 @@ inline bool MoneyRange(const CAmount& nValue) { return (nValue >= 0 && nValue <=
37 class CFeeRate
38 {
39 private:
40-    CAmount nSatoshisPerK; // unit is satoshis-per-1,000-bytes
41+    CAmount nSatoshisPerK; //! unit is satoshis-per-PRECISION_MULTIPLIER-bytes

MarcoFalke commented at 10:40 am on April 2, 2016:

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 commented at 10:49 am on April 2, 2016:

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 commented at 11:05 am on April 2, 2016:

Fine, then just remove PRECISION_MULTIPLIER.

jtimon commented at 10:54 am on April 2, 2016: contributor

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 commented at 1:27 pm on April 3, 2016: member

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 commented at 1:33 pm on April 3, 2016: contributor

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 commented at 1:38 pm on April 3, 2016: member

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

jtimon commented at 11:57 am on April 6, 2016: contributor

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

MarcoFalke commented at 3:52 pm on April 6, 2016: 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 force-pushed on Apr 14, 2016

jtimon commented at 2:35 pm on April 14, 2016: contributor

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 ( https://github.com/bitcoin/bitcoin/pull/7728/commits/5ca24733d08ac57e01c521ded864fd516448f351 ) but I really don’t understand what is happening here. In the same way, the fisrt commit fails without the fixup after it.

Trivial: Fees: Add KB=1000 constant 9dca2d592d

in src/amount.cpp: in 5ca24733d0 outdated

14@@ -15,7 +15,7 @@ CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nBytes_)
15     int64_t nSize = int64_t(nBytes_);
16 
17     if (nSize > 0)
18-        nSatoshisPerK = nFeePaid * 1000 / nSize;
19+        nSatoshisPerK = nFeePaid * PRECISION_MULTIPLIER / nSize;

MarcoFalke commented at 3:17 pm on April 14, 2016:

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 commented at 5:19 pm on April 14, 2016:

Yes, it seems that was it, https://github.com/bitcoin/bitcoin/pull/7728/commits/4cb815a8f7c5422a6c5f4d020f027aefe7ffe172 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 force-pushed on Apr 14, 2016

jtimon commented at 5:34 pm on April 14, 2016: contributor

Removed the PRECISION_MULTIPLIER=KB constant.

Fees: Tests: Check CFeeRate internal precision in mempool_tests.cpp ddca6423f5

Fees: Tests: Check CFeeRate full constructor in amount_tests.cpp d5eed84ef7

in src/test/mempool_tests.cpp: in 3e4d00d107 outdated

565@@ -566,11 +566,19 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
566     BOOST_CHECK_EQUAL(pool.GetMinFee(pool.DynamicMemoryUsage() * 9 / 2).GetFeePerK(), (maxFeeRateRemoved.GetFeePerK() + 1000)/8);
567     // ... with a 1/4 halflife when mempool is < 1/4 its target size
568 
569-    SetMockTime(42 + 7*CTxMemPool::ROLLING_FEE_HALFLIFE + CTxMemPool::ROLLING_FEE_HALFLIFE/2 + CTxMemPool::ROLLING_FEE_HALFLIFE/4);
570+    unsigned i = 5;
571+    while (pool.GetMinFee(1).GetFeePerK() > (int)(2 * KB)) {
572+        SetMockTime(42 + (++i)*CTxMemPool::ROLLING_FEE_HALFLIFE + CTxMemPool::ROLLING_FEE_HALFLIFE/2 + CTxMemPool::ROLLING_FEE_HALFLIFE/4);
573+    }
574+    // test increased stored precission in CFeeRate

MarcoFalke commented at 7:23 pm on April 29, 2016:

nit: typo

in src/test/mempool_tests.cpp: in 3e4d00d107 outdated

565@@ -566,11 +566,19 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
566     BOOST_CHECK_EQUAL(pool.GetMinFee(pool.DynamicMemoryUsage() * 9 / 2).GetFeePerK(), (maxFeeRateRemoved.GetFeePerK() + 1000)/8);
567     // ... with a 1/4 halflife when mempool is < 1/4 its target size
568 
569-    SetMockTime(42 + 7*CTxMemPool::ROLLING_FEE_HALFLIFE + CTxMemPool::ROLLING_FEE_HALFLIFE/2 + CTxMemPool::ROLLING_FEE_HALFLIFE/4);
570+    unsigned i = 5;
571+    while (pool.GetMinFee(1).GetFeePerK() > (int)(2 * KB)) {

MarcoFalke commented at 7:23 pm on April 29, 2016:

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 commented at 7:38 pm on April 29, 2016:

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 commented at 6:38 pm on May 2, 2016:

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 commented at 10:06 am on May 20, 2016:

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 .

jtimon force-pushed on May 20, 2016

sipa commented at 10:37 am on June 3, 2016: member

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

jtimon commented at 1:09 pm on August 30, 2016: contributor

ping

MarcoFalke commented at 11:30 am on September 9, 2016: 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 commented at 6:19 am on September 10, 2016: member

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

MarcoFalke commented at 11:08 am on September 11, 2016: 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 commented at 4:32 pm on September 11, 2016: contributor

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 commented at 4:36 pm on September 12, 2016: member

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 commented at 8:38 pm on September 12, 2016: contributor

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 commented at 4:01 pm on September 13, 2016: member

Closing for now per above discussion.

MarcoFalke closed this on Sep 13, 2016

DrahtBot locked this on Sep 8, 2021

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