Page MenuHomePhabricator

Remove unused param from txmempool::estimateFee and estimatefee RPC

Authored by jasonbcox on Feb 13 2019, 19:18.


Group Reviewers
Restricted Project

Cleanup estimatefee. This helps simplify the coincontrol logic being backported in T521.

Depends on D2580

Test Plan

ninja check check-functional

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 5031
Build 8125: Bitcoin ABC Buildbot (legacy)
Build 8124: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
790 ↗(On Diff #7325)

nBlocksToConfirm should probably also be cleaned up.

16 ↗(On Diff #7325)

Remove nConfirmTarget then.

This revision now requires changes to proceed.Feb 15 2019, 15:56
Fabien requested changes to this revision.Feb 15 2019, 16:06
Fabien added inline comments.
777 ↗(On Diff #7325)

Shouldn't it be > 0, or is it intentional to allow for using the deprecated parameter without returning the help message ?
If the later is correct this can be confusing to user. If your argument is accepted with no error you can expect it to be taken into account. And if the help showing the deprecation doesn't show up, you may never notice that the argument is actually deprecated.

783 ↗(On Diff #7325)

More thought: as it is not used anyway, is it really necessary to mark it as deprecated rather than deleting it ? Leaving it would require the help message to be cleaned up later to remove it.

16 ↗(On Diff #7325)

Do you plan to remove nConfirmTarget also (no longer used) ?

jasonbcox added inline comments.
783 ↗(On Diff #7325)

Originally I was going to clean it up later. But after thinking about your comment above, I think we can just provide a message like:

if (request.params.size() == 1) {
    // DEPRECATED error message
790 ↗(On Diff #7325)

It's used in the labelFeeEstimation below. I can still clean this up, but the message will need to change to "next block".

jasonbcox added inline comments.
16 ↗(On Diff #7325)

I will do this in a separate diff due to the size of that change being larger than this one.

jasonbcox marked an inline comment as done.

Cleanup nBlocksToConfirm and change nblocks argument deprecation implementation

deadalnix requested changes to this revision.Feb 19 2019, 20:02
deadalnix added inline comments.

This is breaking that API with no test and no mention in the release notes and no deprecation cycle.

This revision now requires changes to proceed.Feb 19 2019, 20:02


  • deprecation facility
  • release notes
  • tests
Fabien requested changes to this revision.Feb 20 2019, 08:40
Fabien added inline comments.
788 ↗(On Diff #7417)

Unless I'm missing something, you can avoid this by moving blocks of code like this:

  • first check request.params.size() > 1, and return with help message if false
  • second check `(request.params.size() == 1) && !IsDeprecatedRPCEnabled(gArgs, "estimatefee"), and return as well if false
  • lastly return ValueFromAmount(g_mempool.estimateFee().GetFeePerK());
22 ↗(On Diff #7417)

Is it safe to compare floats with equality in python ?

This revision now requires changes to proceed.Feb 20 2019, 08:40
788 ↗(On Diff #7417)


22 ↗(On Diff #7417)

Same issue as in many languages still applies, but this value happens to have no rounding error. That said, it's still best to fix this.

Fixed logic flow + added assert_equal_float for precision checking

16 ↗(On Diff #7325)
Fabien requested changes to this revision.Feb 22 2019, 08:00
Fabien added inline comments.
22 ↗(On Diff #7434)

You may have over-engineered it here:

from decimal import Decimal
assert_equal(self.nodes[0].estimatefee(), Decimal('0.00001'))
This revision now requires changes to proceed.Feb 22 2019, 08:00

Rebase + don't overengineer casting

Removed assert_equal_float as it's no longer necessary

deadalnix requested changes to this revision.Feb 23 2019, 00:59

So it does looks like that you clearly have a diff for the RPC specifically that detaches itself.

In it, you remove some parameter, but you also unmap the parameter name, which is going to break anything using named parameters, even when the deprecation flags are used.

808 ↗(On Diff #7443)

It seems like there is some behavior change here. What was this behavior for and why does it need to be removed?

This revision now requires changes to proceed.Feb 23 2019, 00:59

Fixed named arguments + added a test for it

808 ↗(On Diff #7443)

This appears to be a dead codepath since we've done a number of modifications to txmempool::estimateFee(). See the current implementation:

CFeeRate CTxMemPool::estimateFee() const {

    uint64_t maxMempoolSize =
        gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000;
    // minerPolicy uses recent blocks to figure out a reasonable fee.  This
    // may disagree with the rollingMinimumFeerate under certain scenarios
    // where the mempool  increases rapidly, or blocks are being mined which
    // do not contain propagated transactions.
    return std::max(GetConfig().GetMinFeePerKB(), GetMinFee(maxMempoolSize));

Although I suppose it's possible to force this function to return zero with certain arguments, I don't see why a user would do such a thing. As such, it seemed prudent to clean up the implementation.

deadalnix requested changes to this revision.Feb 25 2019, 23:08

This is still doing many things at once. Changing the API of the RPC is more than enough for its own patch for instance.

This revision now requires changes to proceed.Feb 25 2019, 23:08

Split into the following diffs:
D2613 - Add initial rpc_estimatefee test
D2614 - Refactor nblocks internally
D2615 - RPC API changes