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.

Sign up for GitHub

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

Jump to bottom

[Core] New fee estimation code #1641

Merged
merged 6 commits into from
Jun 2, 2020
Merged

[Core] New fee estimation code #1641

merged 6 commits into from
Jun 2, 2020

Conversation

random-zebra
Copy link

This introduces the new fee and priority estimation code described here https://www.mail-archive.com/bitcoin-development@lists.sourceforge.net/msg06405.html.

Instead of calculating the median fee for each possible number of blocks needed to confirm, the new code divides the possible fee rates into buckets (spaced logarithmically) and keeps track of the number of blocks needed to confirm for each transaction in each bucket.

Backports:

except for the functional test (smartfees.py) which is based on an older version of the framework.
Instead I've restored a more recent feature_fee_estimation.py (commenting out the check for estimatesmartfee which can be reintroduced once bitcoin#6134 is backported).

Additional commits exclude zerocoins txes from the estimates calculation, as they have fixed fee/priority.

@random-zebra
Create new BlockPolicyEstimator for fee estimates
b67049b 
This class groups transactions that have been confirmed in blocks into
buckets, based on either their fee or their priority.  Then for each
bucket, the class calculates what percentage of the transactions were
confirmed within various numbers of blocks.  It does this by keeping an
exponentially decaying moving history for each bucket and confirm block
count of the percentage of transactions in that bucket that were
confirmed within that number of blocks.

-Eliminate txs which didn't have all inputs available at entry from
fee/pri calcs

-Add dynamic breakpoints and tracking of confirmation delays in mempool
transactions

-Remove old CMinerPolicyEstimator and CBlockAverage code

-Pass a flag to the estimation code, using IsInitialBlockDownload as a
proxy for when we are still catching up and we shouldn't be counting how
many blocks it takes for transactions to be included.

-Add a policyestimator unit test
@random-zebra
PolicyEstimator: exclude zerocoin spends when computing estimates
8ebcbcd
@random-zebra
[Trivial] Remove redundat check in ContainsZerocoins
a8fa99b 
HasZerocoinSpendInputs returns true for either public or private zc
spends
@random-zebra
[Trivial] Fix typos
c5c9df0
@random-zebra
[qt] Properly display required fee instead of minTxFee
988f349 
backports bitcoin/bitcoin@abd8b76 and
bitcoin/53238ff0b1085352e4aaa796d0e473551e573143
@random-zebra
[QA] Restore 'feature_fee_estimation.py' functional test
4038de4 
commenting out the smartFee checks for now
@random-zebra random-zebra added this to the 5.0.0 milestone May 23, 2020
@random-zebra random-zebra self-assigned this May 23, 2020
@ambassador000
Copy link

Not sure if I'm testing it right, but some feedback in case if it's usable:

Generally, it is working as intended. Transactions are smoothly going through with the After Fee amount copy-pasted. Total remaining from the selected UTXO is sometimes higher than 0.00 PIV (although I guess this PR is not even intended to keep it at 0.00 PIV). Furthermore, some feedback:

Initial Custom Fee is set to 0.002 tPIV/kb:
newFew01

When Custom Fee is being manually updated to 0.003 tPIV/kb, as visible on screenshot below Total remaining from the selected UTXO stays at 0.20 tPIV. In other words, it is not being updated in real time...
newFew02

...which is visible on this screenshot because transaction creation failed with slightly increased Custom Fee:
newFew03

@random-zebra
Copy link
Author

random-zebra commented May 24, 2020 •
edited
Loading

Thanks for the feedback @NoobieDev12.
This PR is only about the suggested fee estimation.
Testing it, would ideally mean leaving the wallet running for several hours (in order to compute the estimates) and then using estimatefee or changing the suggested fee ("Fast/Normal/Slow") in the GUI.
At the moment, though, this cannot be done on mainnet or testnet, as we have a very low number of transactions, every tx gets included in the next block (and there is not enough datapoints to compute the estimates). As a result, the rpc returns -1 and the GUI always suggests minTxFee (usually 0.0001 PIV), which makes sense: there is no need to pay a higher fee when mempool and blocks are mostly empty.

Unit and functional tests have been added to verify the execution of the new estimation code under different conditions.
feature_fee_estimation.py sends hundreds of txes to the mempool and mines them with two stingy miners (creating blocks of at most 9 and 18 kB), prints the estimations at various block ranges and verify some invariants on them.

As for the specific issues you highlight:

  • Total remaining from the selected UTXO is sometimes higher than 0.00

yep, not related: this is due to the bug fixed by #1611

  • In other words, it is not being updated in real time...

It is not meant to be. When you change the fee you are still sending the same amount, from the same inputs, so you have the same Total remaining from the selected UTXO.
What is changing is the portion of this Total remaining that will be actually sent back as change vs the portion that will be used as fee.
If you want to select the new After Fee amount, you would need to reopen Coin Control after changing the fee.

@random-zebra random-zebra mentioned this pull request May 25, 2020
Merged
1 task
@Fuzzbawls
Copy link
Collaborator

Concept ACK, and code looks ok. going to spool up a client to leave running overnight to check runtime effect (not hoping for much, but doesn't hurt)

Fuzzbawls
Fuzzbawls approved these changes Jun 2, 2020
View reviewed changes
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.

ACK 4038de4

@furszy furszy added this to Ready in perpetual updating PIVX Core to BTC Core via automation Jun 2, 2020
furszy
furszy approved these changes Jun 2, 2020
View reviewed changes
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

ACK 4038de4 and merging

@furszy furszy merged commit 7c10214 into PIVX-Project:master Jun 2, 2020
perpetual updating PIVX Core to BTC Core automation moved this from Ready to Done Jun 2, 2020
random-zebra added a commit that referenced this pull request Jun 3, 2020
@random-zebra
Merge #1645: [Core] Implement accurate memory accounting for mempool
4986708 
368665e Implement accurate memory accounting for mempool (random-zebra)

Pull request description:

  Based on top of:
  - [x] #1641

  This continues the work of #1531 adding memory accounting for the mempool.
  Backports bitcoin#6410.

  Original description:
  > This implements accurate memory usage accounting for the mempool. It is only exposed through getmempoolinfo for now, but could be used for limiting the resource requirements too (bitcoin#6281).

ACKs for top commit:
  furszy:
    pretty nice, tested ACK 368665e
  Fuzzbawls:
    ACK 368665e

Tree-SHA512: f1dd0e98af58133255db02ae57f20c5d1c0b210610bf6e6c99a112c8c74c0e83e0ae05fd22a933cc4db0eaca36b5f45fa27231879809b348ba0dba034e176767
@random-zebra random-zebra mentioned this pull request Aug 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@furszy furszy furszy approved these changes

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

Assignees

@random-zebra random-zebra

Labels
GUI Tests TX fees and policy Upstream
Projects
perpetual updating PIVX Core to BTC Core
  
Done
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@random-zebra @ambassador000 @Fuzzbawls @furszy