Page MenuHomePhabricator

Update mempool fee estimator to factor in rollingMinimumFeerate
ClosedPublic

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

Details

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
estimate-fee-fix
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3869
Build 5811: Bitcoin ABC Buildbot (legacy)
Build 5810: arc lint + arc unit

Event Timeline

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

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 added inline comments.
src/test/policyestimator_tests.cpp
300 ↗(On Diff #5742)

Congrats, you saved 0 lines of code.

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.

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.

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 added inline comments.
src/test/policyestimator_tests.cpp
296
  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.

This revision is now accepted and ready to land.Jan 3 2019, 23:05
Fabien added a reviewer: schancel.
This revision was automatically updated to reflect the committed changes.