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

rpc method estimatefee #735

Closed
wants to merge 5 commits into from

Conversation

DanielKrawisz
Copy link
Contributor

@DanielKrawisz DanielKrawisz commented Aug 13, 2016

This pr implements estimatefee similar to the way it works in Bitcoin Core, as described here. estimatefee allows for one parameter, NumBlocks. It attempts to provide an estimated fee / kb rate (in Bitcoins) for a transaction to be confirmed before at least NumBlocks (inclusive) have been mined.

Every time a transaction is added to the mempool, it is registered with the fee estimator. Every time a new block is received, it is also registered with the fee estimator. The fee estimator keeps 25 bins containing information on transactions for which 1 to 25 blocks have been mined before being put in a block. Each bin contains information on up to 100 transactions.

Let m be a bijective map from all sots in all bins to the set if indices from 1 to N, where N is the number of transactions in all bins. m maps the first slot in the first bin to 1, the second to 2, and so on, all the way through all the bins. The value returned by estimatefee is the fee / kb value of the transactions whose index in a sorted list of all transactions (with the highest value first) is the same as m[s], where s is the middle slot in bin NumBlocks. estimatefee therefore is a monotonically decreasing function of NumBlocks.

It is also possible to rollback the effect of a limited number of blocks in case of orphans. Default number of rollbacks allowed is 2.

Since it is very easy to store 25 values, all results for all values of NumBlocks are cached when estimatefee is first called. Each time a new block is registered or rolled back, the cache is deleted.

In core, the set of observed transactions is saved between sessions in a file that also contains mempool transactions. btcd doesn't seem to have a similar file, so fee estimation starts over if the program restarts.

The server saves the state of the FeeEstimator on shutdown into the database and tries to restore it upon initialization. If it cannot, then it creates a new FeeEstimator.

A corresponding pr to btcrpcclient has been made here.

estimatefee.go Outdated
)

const (
// estimateFeeDepth is the maximum number of blocks before a transaction
Copy link
Member

Choose a reason for hiding this comment

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

The variable name as declared and what's referenced in the comment are mismatched. The comment should be updated to reference estimateFeeBins instead of estimateFeeDepth.

@Roasbeef
Copy link
Member

Roasbeef commented Aug 15, 2016

Thanks for the PR!

Great work, we've been missing a fee estimation model within btcd for too long now. I've been meaning to revisit a project of mine which predicts transaction fees, but never got around to doing it. The method is a bit more complex (it uses Gradient Boosted Regression Trees), and hasn't yet been compared properly to the existing fee estimation methods so I've held off on proposing integration of the project into btcd.

I've done an initial pass through the PR, but haven't closely examined the tests yet nor verified the correctness of the model.

One recommendation: I think fees should be stored and predicated in sat/byte rather than btc/kilobyte. The former unit should yield more accurate fee estimations as its a more precise unit. Additionally, it avoids the tendency of a fee/KB model to cause users to over pay due to the rounding involved in the calculations.

btcd doesn't seem to have a similar file, so fee estimation starts over if the program restarts.

No such file currently exists, however it should be rather straight forward to persist the state of the fee estimation model to the database. A bucket within the metadata storage can be dedicated to storing the fee estimation data so it persists between restarts.

Also if we decide to move forward with this method, then it'll need a corresponding PR within btcrpcclient so users can programmatically gain access to the new RPC call.

@Roasbeef
Copy link
Member

Roasbeef commented Aug 15, 2016

Does this model incorporate Alex Morcos' modifications to Gavin's initial model? At first glance, it doesn't appear that it does.

blockmanager.go Outdated
@@ -1386,6 +1396,7 @@ func (b *blockManager) Pause() chan<- struct{} {
// newBlockManager returns a new bitcoin block manager.
// Use Start to begin processing asynchronous block and inv updates.
func newBlockManager(s *server, indexManager blockchain.IndexManager) (*blockManager, error) {

Copy link
Member

Choose a reason for hiding this comment

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

Looks like an extra blank line snuck in here.

@davecgh
Copy link
Member

davecgh commented Aug 19, 2016

This will require a rebase now that #737 went in. I apologize for the churn, but it was important to bite the bullet and get the mempool separated.

estimatefee.go Outdated
bin := ef.bin[blocksToConfirm]

// Remove a random element and replace it with this new tx.
if len(bin) == int(ef.binSize) {
Copy link
Member

Choose a reason for hiding this comment

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

ef.binSize is already an int. No need to cast it.

@dajohi
Copy link
Member

dajohi commented Oct 27, 2016

@DanielKrawisz can you rebase one more time and i will run/test it.

@davecgh
Copy link
Member

davecgh commented Oct 30, 2016

I haven't reviewed the updated code yet, but this is failing the CI builds.

++go fmt ./addrmgr/... ./blockchain/... ./btcec/... ./btcjson/... ./chaincfg/... ./cmd/... ./connmgr/... ./database/... ./integration/... ./limits/... ./mempool/... ./mining/... ./peer/... ./rpctest/... ./txscript/... ./wire/... .
mempool/mempool.go
server.go

Looks like you need to run go fmt.

@DanielKrawisz
Copy link
Contributor Author

I forgot to run goclean.sh after the last rebase!

@Roasbeef
Copy link
Member

Testing this PR on mainnet now. Will report back with findings/graphs/comparisons.

@stevenroose
Copy link
Contributor

Fixes #139

@jlopp
Copy link

jlopp commented Feb 24, 2017

Bump this is a pretty important PR; it's sad to see it languish.

@Roasbeef
Copy link
Member

@DanielKrawisz can you squash those additional commits down into a core set? Thanks!

DanielKrawisz and others added 5 commits November 14, 2017 01:58
of how long it takes for unconfirmed txs to be mined into blocks. It
can also roll itself back in the case of an orphan block.
…mprovements.

Rollback takes a block hash rather than a BlockStamp.

Increase rounds in TestEstimateFeeRollback to test dropping txs that have been in the mempool too long.
new txs that it observes. The block manager alerts the fee estimator
of new and orphaned blocks.

Check for invalid state and recreate FeeEstimator if necessary.
and the server searches the database for a previous state to load
when the program is turned on.
@Roasbeef
Copy link
Member

Roasbeef commented Dec 1, 2017

I've been running this on testnet for a few days now, and hit this earlier today:

2017-12-01 03:34:47.900 [INF] CHAN: REORGANIZE: Block 00000000000033e8d1f0118d3504b2e0580f5ed603a76de8d5979106585c602a is causing a reorganize.                                                                                                 [1442/1788]
fatal error: sync: Unlock of unlocked RWMutex

goroutine 26 [running]:
runtime.throw(0xab12ad, 0x20)
        /usr/lib/go-1.8/src/runtime/panic.go:596 +0x95 fp=0xc43db28f48 sp=0xc43db28f28
sync.throw(0xab12ad, 0x20)
        /usr/lib/go-1.8/src/runtime/panic.go:585 +0x35 fp=0xc43db28f68 sp=0xc43db28f48
sync.(*RWMutex).Unlock(0xc4201fc1fc)
        /usr/lib/go-1.8/src/sync/rwmutex.go:118 +0xa7 fp=0xc43db28f98 sp=0xc43db28f68
runtime.call32(0x0, 0xbee280, 0xc43c07e1f0, 0x800000008)
        /usr/lib/go-1.8/src/runtime/asm_amd64.s:514 +0x48 fp=0xc43db28fc8 sp=0xc43db28f98
panic(0x9ecc40, 0xeadc70)
        /usr/lib/go-1.8/src/runtime/panic.go:489 +0x2cf fp=0xc43db29060 sp=0xc43db28fc8
runtime.panicindex()
        /usr/lib/go-1.8/src/runtime/panic.go:28 +0x5e fp=0xc43db29080 sp=0xc43db29060
github.com/roasbeef/btcd/mempool.(*FeeEstimator).RegisterBlock(0xc4204f2b00, 0xc43c3b3c70, 0x0, 0x0)
        /root/go/src/github.com/roasbeef/btcd/mempool/estimatefee.go:283 +0xc11 fp=0xc43db29460 sp=0xc43db29080
github.com/roasbeef/btcd/netsync.(*SyncManager).handleBlockchainNotification(0xc42013e370, 0xc43b9a5220)
        /root/go/src/github.com/roasbeef/btcd/netsync/manager.go:1257 +0x210 fp=0xc43db29530 sp=0xc43db29460
github.com/roasbeef/btcd/netsync.(*SyncManager).(github.com/roasbeef/btcd/netsync.handleBlockchainNotification)-fm(0xc43b9a5220)
        /root/go/src/github.com/roasbeef/btcd/netsync/manager.go:1456 +0x34 fp=0xc43db29550 sp=0xc43db29530
github.com/roasbeef/btcd/blockchain.(*BlockChain).sendNotification(0xc4201fc180, 0x1, 0xa4a2c0, 0xc43c3b3c70)
        /root/go/src/github.com/roasbeef/btcd/blockchain/notifications.go:78 +0xc0 fp=0xc43db29598 sp=0xc43db29550
github.com/roasbeef/btcd/blockchain.(*BlockChain).connectBlock(0xc4201fc180, 0xc42a85aaf0, 0xc43c3b3c70, 0xc43c562240, 0xc43062ca00, 0x23, 0x23, 0x0, 0x0)
        /root/go/src/github.com/roasbeef/btcd/blockchain/chain.go:679 +0x402 fp=0xc43db29640 sp=0xc43db29598
github.com/roasbeef/btcd/blockchain.(*BlockChain).reorganizeChain(0xc4201fc180, 0xc42ac66480, 0xc42ac66450, 0xc4335f42f0, 0x1)
        /root/go/src/github.com/roasbeef/btcd/blockchain/chain.go:997 +0x152f fp=0xc43db29818 sp=0xc43db29640
github.com/roasbeef/btcd/blockchain.(*BlockChain).connectBestChain(0xc4201fc180, 0xc42a85b110, 0xc43bff8460, 0xc400000000, 0x0, 0x0, 0x79e49b)
        /root/go/src/github.com/roasbeef/btcd/blockchain/chain.go:1113 +0x9ea fp=0xc43db29908 sp=0xc43db29818
github.com/roasbeef/btcd/blockchain.(*BlockChain).maybeAcceptBlock(0xc4201fc180, 0xc43bff8460, 0xc400000000, 0xc43c3102e8, 0x79d47b, 0xc4201fc1fc)
        /root/go/src/github.com/roasbeef/btcd/blockchain/accept.go:78 +0x220 fp=0xc43db299d0 sp=0xc43db29908
github.com/roasbeef/btcd/blockchain.(*BlockChain).processOrphans(0xc4201fc180, 0xc43c4c7240, 0xffffffff00000000, 0x0, 0x0)
        /root/go/src/github.com/roasbeef/btcd/blockchain/process.go:118 +0x319 fp=0xc43db29ac0 sp=0xc43db299d0
github.com/roasbeef/btcd/blockchain.(*BlockChain).ProcessBlock(0xc4201fc180, 0xc4330fb9d0, 0xc400000000, 0xc4306e0000, 0x0, 0x0)
        /root/go/src/github.com/roasbeef/btcd/blockchain/process.go:236 +0x368 fp=0xc43db29c38 sp=0xc43db29ac0
github.com/roasbeef/btcd/netsync.(*SyncManager).handleBlockMsg(0xc42013e370, 0xc43c4c7260)
        /root/go/src/github.com/roasbeef/btcd/netsync/manager.go:569 +0x20d fp=0xc43db29e68 sp=0xc43db29c38
github.com/roasbeef/btcd/netsync.(*SyncManager).blockHandler(0xc42013e370)
        /root/go/src/github.com/roasbeef/btcd/netsync/manager.go:1152 +0x5c3 fp=0xc43db29fd8 sp=0xc43db29e68
runtime.goexit()
        /usr/lib/go-1.8/src/runtime/asm_amd64.s:2197 +0x1 fp=0xc43db29fe0 sp=0xc43db29fd8
created by github.com/roasbeef/btcd/netsync.(*SyncManager).Start
        /root/go/src/github.com/roasbeef/btcd/netsync/manager.go:1374 +0x114

@Roasbeef
Copy link
Member

Roasbeef commented Dec 1, 2017

Here's another instance with a full paste (still haven't pin pointed if it's related to this PR): https://pastebin.com/Qx0t6HZk

@stevenroose
Copy link
Contributor

@DanielKrawisz you still aware of this?

@Roasbeef
Copy link
Member

I've been running this for some time now on my fork of btcd. I haven't hit that panic I listed above in weeks. However, we've added the following commits in order to make the fee estimation more accurate:

@stevenroose
Copy link
Contributor

Hmm, what is the quality of the estimation (I didn't read what algorithm it uses)?

What happened to the ML algos for online estimation that you wrote about? (Read that draft paper this weekend.)

@Roasbeef
Copy link
Member

A slightly modified version of this was recently merged in as part of #1180. It should serve as a starting point to improve the fee estimation algo in the future.

@Roasbeef Roasbeef closed this May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants