Improvements to feeEstimator.

 * Rollback now rolls back a specific block rather than
   the most recent one registered.
 * Parameter minRegisteredBlocks added. An error is
   returned for fee estimation requests if fewer than
   minRegisteredBlocks have been registered.
 * result returned is in bitcoins rather than satoshis.
 * server.EstimateFee() removed. Now just use the feeEstimator
   member var instead.
 * Improve comments.
 * corrections from running goclean.sh

blockmanager.go

@@ -1255,10 +1255,8 @@ func (b *blockManager) handleNotifyMsg(notification *blockchain.Notification) {
 		}
 
 		// Rollback previous block recorded by the fee estimator.
-		// TODO instead of rolling back the previous block,
-		// check whether we are rolling back the correct one!
 		if b.server.feeEstimator != nil {
-			b.server.feeEstimator.Rollback()
+			b.server.feeEstimator.Rollback(block)
 		}
 
 		// Notify registered websocket clients.

estimatefee.go

@@ -40,35 +40,54 @@ type observedTransaction struct {
 	mined    int32 // The block in which this tx was mined.
 }
 
-// tt is safe for concurrent access.
+// registeredBlock
+type registeredBlock struct {
+	hash         *chainhash.Hash
+	transactions []*observedTransaction
+}
+
+// feeEstimator is a class that manages the data necessary to create
+// fee estimations. It is safe for concurrent access.
 type feeEstimator struct {
-	maxRollback     uint32
-	binSize         int
+	maxRollback uint32
+	binSize     int
+
+	// The maximum number of replacements that can be made in a single
+	// bin per block. Default is estimateFeeMaxReplacements
 	maxReplacements int
 
+	// The minimum number of blocks that can be registered with the fee
+	// estimator before it will provide answers.
+	minRegisteredBlocks uint32
+
 	// The last known height.
-	height int32
+	lastKnownHeight int32
 
 	sync.RWMutex
-	observed map[chainhash.Hash]observedTransaction
-	bin      [estimateFeeBins][]*observedTransaction
+	observed            map[chainhash.Hash]observedTransaction
+	bin                 [estimateFeeBins][]*observedTransaction
+	numBlocksRegistered uint32 // The number of blocks that have been registered.
 
 	// The cached estimates.
 	cached []float64
 
 	// Transactions that have been removed from the bins. This allows us to
 	// revert in case of an orphaned block.
-	dropped [][]*observedTransaction
+	dropped []registeredBlock
 }
 
-func NewFeeEstimator(maxRollback uint32) *feeEstimator {
+// newFeeEstimator creates a feeEstimator for which at most maxRollback blocks
+// can be unregistered and which returns an error unless minRegisteredBlocks
+// have been registered with it.
+func newFeeEstimator(maxRollback, minRegisteredBlocks uint32) *feeEstimator {
 	return &feeEstimator{
-		maxRollback:     maxRollback,
-		height:          mempoolHeight,
-		binSize:         estimateFeeBinSize,
-		maxReplacements: estimateFeeMaxReplacements,
-		observed:        make(map[chainhash.Hash]observedTransaction),
-		dropped:         make([][]*observedTransaction, 0, maxRollback),
+		maxRollback:         maxRollback,
+		minRegisteredBlocks: minRegisteredBlocks,
+		lastKnownHeight:     mempoolHeight,
+		binSize:             estimateFeeBinSize,
+		maxReplacements:     estimateFeeMaxReplacements,
+		observed:            make(map[chainhash.Hash]observedTransaction),
+		dropped:             make([]registeredBlock, 0, maxRollback),
 	}
 }
 
@@ -102,12 +121,14 @@ func (ef *feeEstimator) RecordBlock(block *btcutil.Block) {
 	ef.cached = nil
 
 	height := block.Height()
-	if height != ef.height+1 && ef.height != mempoolHeight {
-		panic(fmt.Sprint("intermediate block not recorded; current height is ", ef.height,
+	if height != ef.lastKnownHeight+1 && ef.lastKnownHeight != mempoolHeight {
+		panic(fmt.Sprint("intermediate block not recorded; current height is ", ef.lastKnownHeight,
 			"; new height is ", height))
 	}
 
-	ef.height = height
+	// Update the last known height.
+	ef.lastKnownHeight = height
+	ef.numBlocksRegistered++
 
 	// Randomly order txs in block.
 	transactions := make(map[*btcutil.Tx]struct{})
@@ -120,10 +141,13 @@ func (ef *feeEstimator) RecordBlock(block *btcutil.Block) {
 	var replacementCounts [estimateFeeBins]int
 
 	// Keep track of which txs were dropped in case of an orphan block.
-	dropped := make([]*observedTransaction, 0, 100)
+	dropped := registeredBlock{
+		hash:         block.Hash(),
+		transactions: make([]*observedTransaction, 0, 100),
+	}
 
 	// Go through the txs in the block.
-	for t, _ := range transactions {
+	for t := range transactions {
 		hash := *t.Hash()
 
 		// Have we observed this tx in the mempool?
@@ -150,7 +174,7 @@ func (ef *feeEstimator) RecordBlock(block *btcutil.Block) {
 		if len(bin) == int(ef.binSize) {
 			l := int(ef.binSize - replacementCounts[blocksToConfirm])
 			drop := rand.Intn(l)
-			dropped = append(dropped, bin[drop])
+			dropped.transactions = append(dropped.transactions, bin[drop])
 
 			bin[drop] = bin[l-1]
 			bin[l-1] = &o
@@ -178,13 +202,47 @@ func (ef *feeEstimator) RecordBlock(block *btcutil.Block) {
 	}
 }
 
-// Rollback reverses the effect of the last block on the fee estimator. This
-// can be used in the case of an orphaned block. The maximum number of rollbacks
-// allowed is given by maxRollbacks.
-func (ef *feeEstimator) Rollback() error {
+// Rollback unregisters a recently registered block from the fee estimator.
+// This can be used to reverse the effect of an orphaned block on the fee
+// estimator. The maximum number of rollbacks allowed is given by
+// maxRollbacks.
+//
+// Note: not everything can be rolled back because some transactions are
+// deleted if they have been observed too long ago. That means the result
+// of Rollback won't always be exactly the same as if the last block had not
+// happened, but it should be close enough.
+func (ef *feeEstimator) Rollback(block *btcutil.Block) error {
 	ef.Lock()
 	defer ef.Unlock()
 
+	hash := block.Hash()
+
+	// Find this block in the stack of recent registered blocks.
+	var n int
+	for n = 1; n < len(ef.dropped); n++ {
+		if ef.dropped[len(ef.dropped)-n].hash.IsEqual(hash) {
+			break
+		}
+	}
+
+	if n == len(ef.dropped) {
+		return errors.New("No such block was recently registered.")
+	}
+
+	for i := 0; i < n; i++ {
+		err := ef.rollback()
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// rollback rolls back the effect of the last block in the stack
+// of registered blocks.
+func (ef *feeEstimator) rollback() error {
+
 	// The previous sorted list is invalid, so delete it.
 	ef.cached = nil
 
@@ -195,14 +253,16 @@ func (ef *feeEstimator) Rollback() error {
 		return errors.New("Max rollbacks reached.")
 	}
 
+	ef.numBlocksRegistered--
+
 	dropped := ef.dropped[last]
 	ef.dropped = ef.dropped[0:last]
 
-	// where we are in each bin as we replace txs.
+	// where we are in each bin as we replace txs?
 	var replacementCounters [estimateFeeBins]int
 
 	// Go through the txs in the dropped box.
-	for _, o := range dropped {
+	for _, o := range dropped.transactions {
 		// Which bin was this tx in?
 		blocksToConfirm := o.mined - o.observed - 1
 
@@ -218,7 +278,7 @@ func (ef *feeEstimator) Rollback() error {
 
 			prev := bin[counter]
 
-			if prev.mined == ef.height {
+			if prev.mined == ef.lastKnownHeight {
 				prev.mined = mempoolHeight
 
 				bin[counter] = o
@@ -242,7 +302,7 @@ func (ef *feeEstimator) Rollback() error {
 
 			prev := ef.bin[i][j]
 
-			if prev.mined == ef.height {
+			if prev.mined == ef.lastKnownHeight {
 				prev.mined = mempoolHeight
 
 				ef.bin[i] = append(ef.bin[i][0:j], ef.bin[i][j+1:l]...)
@@ -254,7 +314,7 @@ func (ef *feeEstimator) Rollback() error {
 		}
 	}
 
-	ef.height--
+	ef.lastKnownHeight--
 
 	return nil
 }
@@ -300,7 +360,7 @@ func (b *estimateFeeSet) EstimateFee(confirmations int) float64 {
 		return 0
 	}
 
-	return b.feeRate[(min+max-1)/2]
+	return b.feeRate[(min+max-1)/2] * 1E-8
 }
 
 // newEstimateFeeSet creates a temporary data structure that
@@ -345,21 +405,30 @@ func (ef *feeEstimator) estimates() []float64 {
 
 // Estimate the fee per kb to have a tx confirmed a given number of blocks
 // from now.
-func (ef *feeEstimator) EstimateFee(confirmations uint32) float64 {
+func (ef *feeEstimator) EstimateFee(numBlocks uint32) (float64, error) {
 	ef.Lock()
 	defer ef.Unlock()
 
-	if confirmations <= 0 {
-		return math.Inf(1)
+	// If the number of registered blocks is below the minimum, return
+	// an error.
+	if ef.numBlocksRegistered < ef.minRegisteredBlocks {
+		return -1, errors.New("Not enough blocks have been observed.")
 	}
 
-	if confirmations > estimateFeeBins {
-		return 0
+	if numBlocks == 0 {
+		return -1, errors.New("Cannot confirm transaction in zero blocks.")
+	}
+
+	if numBlocks > estimateFeeBins {
+		return -1, fmt.Errorf(
+			"Can only estimate fees for up to %d blocks from now.",
+			estimateFeeBinSize)
 	}
 
+	// If there are no cached results, generate them.
 	if ef.cached == nil {
 		ef.cached = ef.estimates()
 	}
 
-	return ef.cached[int(confirmations)-1]
+	return ef.cached[int(numBlocks)-1], nil
 }