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

multi: Implement smart fee estimation. #1434

Merged
merged 2 commits into from Dec 6, 2018
Merged

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 matheusd commented Sep 3, 2018

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 decred/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 matheusd:fee-estimator branch 3 times, most recently from 46cf163 to 2922e7c Sep 3, 2018
@davecgh davecgh added this to the 1.4.0 milestone Sep 4, 2018
@dnldd
Copy link
Member

@dnldd 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 matheusd force-pushed the matheusd:fee-estimator branch from 2922e7c to 4264608 Sep 13, 2018
@matheusd
Copy link
Member Author

@matheusd matheusd commented Sep 14, 2018

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
fees/estimator.go Show resolved Hide resolved
fees/estimator.go Show resolved Hide resolved
@matheusd matheusd force-pushed the matheusd:fee-estimator branch 2 times, most recently from 15ef7ab to b0cc3e0 Oct 11, 2018
@matheusd matheusd force-pushed the matheusd:fee-estimator branch 2 times, most recently from 1ff111b to 23cf919 Nov 14, 2018
fees/cmd/dumpfeedb/dumpfeedb.go Outdated Show resolved Hide resolved
fees/cmd/dumpfeedb/dumpfeedb.go Outdated Show resolved Hide resolved
log.go Outdated Show resolved Hide resolved
log.go Outdated Show resolved Hide resolved
log.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
@matheusd matheusd force-pushed the matheusd:fee-estimator branch 2 times, most recently from 5188286 to 863ab8a Dec 4, 2018
Copy link
Member

@davecgh davecgh left a comment

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 Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
bucketFees := make([]feeRate, 0)
prevF := 0.0
for f := float64(cfg.MinBucketFee); f < max; f *= cfg.FeeRateStep {
if (f > 1e5) && (prevF < 1e5) {

This comment has been minimized.

@davecgh

davecgh Dec 6, 2018
Member

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.

This comment has been minimized.

@matheusd

matheusd Dec 6, 2018
Author Member

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).

rpcserver.go Outdated Show resolved Hide resolved
fees/estimator.go Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
fees/estimator.go Outdated Show resolved Hide resolved
@matheusd matheusd force-pushed the matheusd:fee-estimator branch from 6349219 to e12ae0d Dec 6, 2018
dcrjson/chainsvrcmds.go Outdated Show resolved Hide resolved
matheusd added 2 commits Dec 6, 2018
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.
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 matheusd force-pushed the matheusd:fee-estimator branch from efc2236 to 300b684 Dec 6, 2018
@matheusd
Copy link
Member Author

@matheusd 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 approved these changes Dec 6, 2018
@davecgh davecgh merged commit 300b684 into decred:master Dec 6, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matheusd matheusd deleted the matheusd:fee-estimator branch Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants