Page MenuHomePhabricator
Differential D2037

Update mempool fee estimator to factor in rollingMinimumFeerate
ClosedPublic
Actions

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

Details

Reviewers
deadalnix
schancel
Group Reviewers
Restricted Project
Commits
rSTAGING8e11bbd14564: Update mempool fee estimator to factor in rollingMinimumFeerate
rABC8e11bbd14564: Update mempool fee estimator to factor in rollingMinimumFeerate
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

schancel created this revision.Nov 10 2018, 22:46
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 10 2018, 22:46
teamcity added a subscriber: teamcity.Nov 10 2018, 22:48

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Nov 10 2018, 22:50

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3828: Diff 5731!Nov 10 2018, 22:51
schancel updated this revision to Diff 5734.Nov 11 2018, 00:24

Finish patch

teamcity added a comment.Nov 11 2018, 00:25

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

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
teamcity added a comment.Nov 11 2018, 00:34

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3830: Diff 5734.Nov 11 2018, 00:34
schancel updated this revision to Diff 5742.Nov 11 2018, 08:23

Add a test

teamcity added a comment.Nov 11 2018, 08:25

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Nov 11 2018, 08:34

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3836: Diff 5742.Nov 11 2018, 08:34
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

teamcity added a comment.Nov 12 2018, 21:58

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

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
teamcity added a comment.Nov 12 2018, 22:06

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3869: Diff 5778.Nov 12 2018, 22:07
schancel marked an inline comment as done.Nov 13 2018, 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.Nov 13 2018, 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.Nov 25 2018, 17:27
jasonbcox added inline comments.Nov 26 2018, 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.

schancel removed reviewers: deadalnix, jasonbcox.Nov 28 2018, 17:13
deadalnix added inline comments.Nov 28 2018, 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.Nov 28 2018, 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.Dec 3 2018, 06:06

Rebase

teamcity added a comment.Dec 3 2018, 06:08

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Dec 3 2018, 06:17

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4240: Diff 6245.Dec 3 2018, 06:17
schancel added a child revision: D2172: [policy] Delete block policy estimator..Dec 3 2018, 06:28
deadalnix accepted this revision.Jan 3 2019, 23:05
This revision is now accepted and ready to land.Jan 3 2019, 23:05
Fabien commandeered this revision.Jan 4 2019, 15:21
Fabien added a reviewer: schancel.
Closed by commit rABC8e11bbd14564: Update mempool fee estimator to factor in rollingMinimumFeerate (authored by schancel, committed by Fabien). · Explain WhyJan 4 2019, 15:28
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
src/
test/
91 lines
14 lines

Diff 6503

View Options

src/test/policyestimator_tests.cpp

Loading...
View Options

src/txmempool.cpp

Loading...