Skip to content

multi: Implement smart fee estimation. #1434

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
Merged
merged 2 commits into from
Dec 6, 2018
Merged

multi: Implement smart fee estimation. #1434

merged 2 commits into from
Dec 6, 2018
+1,389 −18

Conversation

matheusd
Copy link
Member

@matheusd matheusd commented Sep 3, 2018

Initial release of the fee estimator.

Closes #1412

@matheusd
Copy link
Member Author

matheusd commented Sep 3, 2018 •
edited
Loading

Sending for initial review and to discuss some last points that might require working before merging.

As a preliminary, I'm sending attached the fee database collected over the last few days in mainnet, and the dump of the estimator state for this same database:

feesdb-mainnet.tar.gz
fees.txt

Results of the estimation:

$ ./dcrctl.sh estimatefee 1
-32603: no bucket with the minimum required success percentage found
$ ./dcrctl.sh estimatefee 2
0.00112764
$ ./dcrctl.sh estimatefee 3
0.00112764
$ ./dcrctl.sh estimatefee 4
0.00087775
$ ./dcrctl.sh estimatefee 6
0.00087775

A few notes for discussion:

  • I've hooked into the estimatefee instead of the estimatesmartfee because there are no parameters to the estimator right now (eg: conservative vs economical) and the handler was already there, I just had to hook it. Can rework into a brand new handler if that is preferable.

  • Those txs with lower fee are already bringing down the estimates vs the current default wallet fee. Eyeballing from logs, they seem to be all revocations, so unsure whether we'll need to special case them.

  • The 0.00108347 bucket is mostly filled with tickets, which due to hard coded size estimates end up paying fees slightly larger than strictly required. Hopefully fixing Improve size/fee estimation for tickets dcrwallet#1272 improves this.

  • Due to how tickets are managed in the mempool and the fact that they are sent along with split txs (but are only eligible to be included in the block after the split has been mined and validated), they also severely skew the 1 confirmation bucket. Ticket handling in mempool would have to be seriously tweaked to fix this. Specifically, tickets would need to be tracked as out of the mempool (either as orphans or as some other classification) and could only enter the mempool after their respective split had been mined.

@matheusd matheusd force-pushed the fee-estimator branch 3 times, most recently from 46cf163 to 2922e7c Compare September 3, 2018 14:35
@davecgh davecgh added this to the 1.4.0 milestone Sep 4, 2018
@dnldd
Copy link
Member

dnldd commented Sep 10, 2018

Nicely done @matheusd, numblocks parameter description for estimatefee will have to be updated if the current rpc setup is maintained. Leaving this here as a reminder.

@matheusd
Copy link
Member Author

Current stats after running in mainnet for ~2 weeks.

Internal estimator state

$ ./dcrctl estimatefee 1
-32603: no bucket with the minimum required success percentage found
$ ./dcrctl estimatefee 2
0.00136971
$ ./dcrctl estimatefee 3
0.00114035
$ ./dcrctl estimatefee 4
0.00114035
$ ./dcrctl estimatefee 5
0.00114035
$ ./dcrctl estimatefee 6
0.00087921
$ ./dcrctl estimatefee 7
0.00087921
$ ./dcrctl estimatefee 16
0.00087921

dajohi
dajohi reviewed Sep 17, 2018
View reviewed changes
dajohi
dajohi reviewed Sep 17, 2018
View reviewed changes
@matheusd matheusd force-pushed the fee-estimator branch 2 times, most recently from 15ef7ab to b0cc3e0 Compare October 11, 2018 15:52
@matheusd matheusd force-pushed the fee-estimator branch 2 times, most recently from 1ff111b to 23cf919 Compare November 14, 2018 14:33
davecgh
davecgh requested changes Dec 4, 2018
View reviewed changes
@matheusd matheusd force-pushed the fee-estimator branch 2 times, most recently from 5188286 to 863ab8a Compare December 4, 2018 16:11
davecgh
davecgh requested changes Dec 6, 2018
View reviewed changes
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

The EstimateSmartFee command in dcrjson needs to be updated to make the second parameter optional as well. Currently it is required.

fees/estimator.go Outdated
bucketFees := make([]feeRate, 0)
prevF := 0.0
for f := float64(cfg.MinBucketFee); f < max; f *= cfg.FeeRateStep {
if (f > 1e5) && (prevF < 1e5) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this should be using a value passed in from elsewhere (perhaps policy.DefaultMinRelayTxFee), especially since wallet should be using 1e4 now. Otherwise this will certainly get out of date.

Copy link
Member Author

Choose a reason for hiding this comment

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

Slight difficulty on this one. I've moved the constant out of the fees package to the server.go file, passing it as an ExtraBucketFee to be inserted.

The main issue with using DefaultMinRelayTxFee is that this is not supposed to be current fee, but rather the one that the majority of the wallets are using (ie, the previous DefaultMinRelayTxFee).

So, one alternative would be to explicitly add a mempool.PreviousDefaultMinRelayTxFee and add comments detailing that it should be bumped once DefaultMinRelayTxFee is also bumped.

Another one, to make this less "magical" would be to remove this constant altogether and add explicit tracking of all order-of-magnitude bucket fees in the range (1e3, 1e4, 1e5, etc).

davecgh
davecgh requested changes Dec 6, 2018
View reviewed changes
davecgh
davecgh requested changes Dec 6, 2018
View reviewed changes
davecgh
davecgh requested changes Dec 6, 2018
View reviewed changes
@matheusd
fees: Add estimator package
0055632 
This adds the first version of the fees package, responsible for
performing fee estimation of network transactions.

The main goal of fee estimation is to allow the usage of dynamic fees
by wallets, contingent on block contention and the desired confirmation
range for a given transaction.

This version was based on bitcoin core fee estimation.
@matheusd
multi: Integrate fee estimation
300b684 
This commit performs the necessary modifications to hook the fee
estimation facility added by the previous commit into the node software.

The block manager is modified to notify the estimator of all new blocks,
such that their transactions can be accounted for, and the mempool is
modified to relay transactions entering and leaving it, so that those
transactions can be tracked.

The results of the estimator can be queried by issuing estimatesmartfee
rpc commands to the node.
@matheusd
Copy link
Member Author

matheusd commented Dec 6, 2018

Squashed. I removed some unnecessary bumps from go.mod and go.sum, but other than that the commits are identical to the previous ones.

@davecgh davecgh changed the title fees: add estimator multi: Implement smart fee estimation. Dec 6, 2018
@davecgh davecgh merged commit 300b684 into decred:master Dec 6, 2018
@matheusd matheusd deleted the fee-estimator branch November 25, 2020 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrick jrick

@dajohi dajohi

@davecgh davecgh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@matheusd @dnldd @jrick @davecgh @dajohi