Page MenuHomePhabricator

Update mempool fee estimator to factor in rollingMinimumFeerate
Needs ReviewPublic

Authored by schancel on Nov 10 2018, 22:46.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

minerPolicyEstimator uses recent blocks to figure out a reasonable fee. This may disagree
with the rollingMinimumFeerate under certain scenarios where the blocks include low fee
transactions, but the mempool is being spammed.

This commit updates the mempool fee estimator to use the maximum of the two values.

Test Plan
make check && ./test/functional/test_runner.py --extended

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D2037 (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4240
Build 6545: Bitcoin ABC Teamcity Staging
Build 6544: arc lint + arc unit

Event Timeline

schancel created this revision.Nov 10 2018, 22:46
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 10 2018, 22:46
schancel updated this revision to Diff 5734.Nov 11 2018, 00:24

Finish patch

deadalnix requested changes to this revision.Nov 11 2018, 00:33
deadalnix added a subscriber: deadalnix.

Tests.

This revision now requires changes to proceed.Nov 11 2018, 00:33
schancel updated this revision to Diff 5742.Nov 11 2018, 08:23

Add a test

jasonbcox requested changes to this revision.Nov 12 2018, 00:27
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/test/policyestimator_tests.cpp
273 ↗(On Diff #5742)

trasnsactions -> transactions

300 ↗(On Diff #5742)

use a for-loop

335 ↗(On Diff #5742)

Comment says 1000. this is 10000. which is correct?

src/txmempool.cpp
971 ↗(On Diff #5742)

Note for future diff: this constant needs to be in bytes instead of MB or at least renamed with ..._SIZE_MB

This revision now requires changes to proceed.Nov 12 2018, 00:27
schancel marked 2 inline comments as done.Nov 12 2018, 17:31
schancel added inline comments.
src/test/policyestimator_tests.cpp
300 ↗(On Diff #5742)

No, then you need crap to figure out which iteration broke.

src/txmempool.cpp
971 ↗(On Diff #5742)

Yeah, it's rather annoying

schancel updated this revision to Diff 5778.Nov 12 2018, 21:56

Address feedback

jasonbcox requested changes to this revision.Nov 12 2018, 22:02
jasonbcox added inline comments.
src/test/policyestimator_tests.cpp
300 ↗(On Diff #5742)

A simple print statement will make this easily debuggable:

for (numBlocks ...) {
    LogPrint("Verifying estimateFee for %s blocks", numBlocks);
    BOOST_CHECK(mpool.GetMinFee(1) <= mpool.estimateFee(numBlocks));
}
src/txmempool.cpp
971 ↗(On Diff #5742)

T478 to fix this later

This revision now requires changes to proceed.Nov 12 2018, 22:02
schancel marked an inline comment as done.Tue, Nov 13, 04:43
schancel added inline comments.
src/test/policyestimator_tests.cpp
300 ↗(On Diff #5742)

Congrats, you saved 0 lines of code.

schancel marked an inline comment as done.Tue, Nov 13, 04:46
schancel added inline comments.
src/test/policyestimator_tests.cpp
300 ↗(On Diff #5742)

I don't agree, and I don't like you blocking me landing code over nits.

schancel requested review of this revision.Sun, Nov 25, 17:27
jasonbcox added inline comments.Mon, Nov 26, 20:15
src/test/policyestimator_tests.cpp
300 ↗(On Diff #5742)

I don't consider this a nit. Bugs arising from copy-pasted code is a well understood phenomenon. Number of lines of code is mostly irrelevant here.

deadalnix added inline comments.Wed, Nov 28, 17:34
src/test/policyestimator_tests.cpp
300 ↗(On Diff #5742)

I don't think any of you have a very strong case here. As @jasonbcox points out, it's not about line of code - the goal never was to produce a test case as compact as possible. But some level of code repetition in tests can actually be beneficial.

See: https://evgenii.com/blog/code-duplication-in-unit-tests/

It wouldn't be outside of tests, but testing code is somewhat different than regular code. I often make the mistake myself of making my test code too dry.

schancel marked an inline comment as done.Wed, Nov 28, 18:02
schancel added inline comments.
src/test/policyestimator_tests.cpp
296 ↗(On Diff #5778)
  1. Case one is special and requires it to be outside the for loop anyways.
  2. If any of these cases blow up you need additional code to figure out what broke.
  3. This isn't a point of correctness, and should be left up to the author of the diff unless there is clearly a readability issue.

I don't believe I *need* a strong case to have *my* code be the way I want it. Rather, the reviewer needs a case as to why they want it changed.

In this case, @jasonbcox thinks it looks cleaner to have a for loop. I disagree. You either need:

for (int i = 1; i < 6; i++) {
    if ( i == 1 ) {
            BOOST_CHECK(CFeeRate(Amount::zero()) == mpool.estimateFee(i), "Failed at block depth " << i);
            continue;
    }
    BOOST_CHECK(mpool.GetMinFee(1) <= mpool.estimateFee(i), "Failed at block depth " << i);
}

Or:

BOOST_CHECK(CFeeRate(Amount::zero()) == mpool.estimateFee(1), "Failed at block depth 1");
for (int i = 2; i < 6; i++) {
    BOOST_CHECK(mpool.GetMinFee(1) <= mpool.estimateFee(i), "Failed at block depth " << i);
}

In one case you have a nasty anti-pattern and unexpected loop boundaries. In the other you break up the logical grouping of what's being tested. In both cases, what I have here looks better, reads better, and is easier to debug given the output from the boost testing framework.

Precisely what clarity is being provided by having a for loop here? What is being improved?

And yes, I've spent more time writing this than "fixing" my diff. The reason is I think that this review process is borked.

schancel updated this revision to Diff 6245.Mon, Dec 3, 06:06

Rebase