Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve usage of fee estimation code #1787

Merged
merged 9 commits into from
Aug 18, 2020

Conversation

furszy
Copy link

@furszy furszy commented Aug 4, 2020

Another back port, coming from dashpay#6134 .

When automatically adding a fee estimate to a transaction and in the event no estimate is available for the desired confirmation target, it makes more sense to default to a fee estimate for a worse confirmation target rather than the hard coded minimum.

TODO: Left to back port smartfees.py.

We need it to be able to move forward in #1726

morcos and others added 7 commits August 4, 2020 02:55
These are more useful fee and priority estimation functions. If there is no fee/pri high enough for the target you are aiming for, it will give you the estimate for the lowest target that you can reliably obtain.  This is better than defaulting to the minimum.  It will also pass back the target for which it returned an answer.
coming from btc@4fe28236c0c16e20ddd539f38fc8d58db5eb83ed
This provides more conservative estimates and reacts more quickly to a backlog.
Unfortunately the unit test for fee estimation depends on the success threshold (and the decay) chosen; also modify the unit test for the new default success thresholds.
coming from btc@56106a3300f844afcadf6dce50d5ef1d337f50b9
@furszy furszy self-assigned this Aug 4, 2020
@random-zebra
Copy link

There is currently a failure in the policyestimator unit test at line 194

BOOST_CHECK(mpool.estimatePriority(i) < origPriEst[i-1] - deltaPri);

(e.g. see https://travis-ci.com/github/PIVX-Project/PIVX/jobs/368134714)

This happens at i=8 and is probably related to a previous known problem: estimatePriority(8) returns exactly 1e9, instead of a numberbetween 1e8 and 1e9 - deltaPri as expected (that's why line 120 is commented out).

I wouldn't dig too much on the issue though, as the priority estimation is going to be completely removed (bitcoin#7730)


For the functional test, as reported in the description of #1641, the smartfee.py test was skipped on purpose, as it is based on a very old version of the framework.
It was replaced by feature_fee_estimation.py (where the smartfee test was commented out, specifically waiting for this backport of bitcoin#6134).

I've un-commented it in 245c3df
It passes.

This check currently fails.
As the whole priority estimation code will be removed soon (PR 1788),
let's just skip it for now.
@furszy furszy changed the title [WIP] Improve usage of fee estimation code Improve usage of fee estimation code Aug 4, 2020
@furszy
Copy link
Author

furszy commented Aug 4, 2020

Awesome @random-zebra 👌 , removed the WIP tag from the title. And agree on the removal.

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 3aded91

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to the RPC need to be documented (along with #1788), tagging "Needs release notes".
ACK 3aded91 and merging...

perpetual updating PIVX Core to BTC Core automation moved this from In Progress to Ready Aug 18, 2020
@random-zebra random-zebra merged commit 4b1f3eb into PIVX-Project:master Aug 18, 2020
perpetual updating PIVX Core to BTC Core automation moved this from Ready to Done Aug 18, 2020
@random-zebra random-zebra added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Aug 18, 2020
random-zebra added a commit that referenced this pull request Aug 23, 2020
2da16f7 [Cleanup] Nuke estimate(smart)priority from the RPC interface (random-zebra)
041f62e [Core] Remove priority estimation (random-zebra)

Pull request description:

  Based on top of
  - [x] #1787

  This removes the functionality behind `estimatepriority` and `estimatesmartpriority` (from bitcoin#7730).

  Instead of just deprecating the relative RPC commands, I think we can outright remove them, as `estimatepriority` was added only in latest minor release (4.2), and `estimatesmartpriority` is introduced in a PR not even merged yet (#1787).

ACKs for top commit:
  Fuzzbawls:
    utACK 2da16f7
  furszy:
    utACK 2da16f7

Tree-SHA512: 7dc964c7627a08eae349df40f40c49bc85761171d97f3d7a828421f6dc46ca86dc7a410c75bd8025b7a684aa09d215a08ac6ae19b64617adac6365cfaee55afc
@random-zebra random-zebra modified the milestones: 5.0.0, 4.3.0 Sep 10, 2020
@Fuzzbawls Fuzzbawls removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Sep 27, 2020
@furszy furszy deleted the 2020_backport_6134 branch November 29, 2022 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants