Comments and improved documentation

3 files changed, 56 insertions, 28 deletions

diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
index 97e9536867..35e2857399 100644
--- a/src/policy/fees.cpp
+++ b/src/policy/fees.cpp
@@ -729,6 +729,9 @@ double CBlockPolicyEstimator::estimateCombinedFee(unsigned int confTarget, doubl
     return estimate;
 }
 
+/** Ensure that for a conservative estimate, the DOUBLE_SUCCESS_PCT is also met
+ * at 2 * target for any longer time horizons.
+ */
 double CBlockPolicyEstimator::estimateConservativeFee(unsigned int doubleTarget) const
 {
     double estimate = -1;
@@ -744,6 +747,13 @@ double CBlockPolicyEstimator::estimateConservativeFee(unsigned int doubleTarget)
     return estimate;
 }
 
+/** estimateSmartFee returns the max of the feerates calculated with a 60%
+ * threshold required at target / 2, an 85% threshold required at target and a
+ * 95% threshold required at 2 * target.  Each calculation is performed at the
+ * shortest time horizon which tracks the required target.  Conservative
+ * estimates, however, required the 95% threshold at 2 * target be met for any
+ * longer time horizons also.
+ */
 CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, int *answerFoundAtTarget, const CTxMemPool& pool, bool conservative) const
 {
     if (answerFoundAtTarget)
diff --git a/src/policy/fees.h b/src/policy/fees.h
index aa179cfddd..d5b63823ad 100644
--- a/src/policy/fees.h
+++ b/src/policy/fees.h
@@ -41,32 +41,39 @@ class TxConfirmStats;
  * within your desired 5 blocks.
  *
  * Here is a brief description of the implementation:
- * When a transaction enters the mempool, we
- * track the height of the block chain at entry.  Whenever a block comes in,
- * we count the number of transactions in each bucket and the total amount of feerate
- * paid in each bucket. Then we calculate how many blocks Y it took each
- * transaction to be mined and we track an array of counters in each bucket
- * for how long it to took transactions to get confirmed from 1 to a max of 25
- * and we increment all the counters from Y up to 25. This is because for any
- * number Z>=Y the transaction was successfully mined within Z blocks.  We
- * want to save a history of this information, so at any time we have a
- * counter of the total number of transactions that happened in a given feerate
- * bucket and the total number that were confirmed in each number 1-25 blocks
- * or less for any bucket.   We save this history by keeping an exponentially
- * decaying moving average of each one of these stats.  Furthermore we also
- * keep track of the number unmined (in mempool) transactions in each bucket
- * and for how many blocks they have been outstanding and use that to increase
- * the number of transactions we've seen in that feerate bucket when calculating
- * an estimate for any number of confirmations below the number of blocks
- * they've been outstanding.
+ * When a transaction enters the mempool, we track the height of the block chain
+ * at entry.  All further calculations are conducted only on this set of "seen"
+ * transactions. Whenever a block comes in, we count the number of transactions
+ * in each bucket and the total amount of feerate paid in each bucket. Then we
+ * calculate how many blocks Y it took each transaction to be mined.  We convert
+ * from a number of blocks to a number of periods Y' each encompassing "scale"
+ * blocks.  This is is tracked in 3 different data sets each up to a maximum
+ * number of periods. Within each data set we have an array of counters in each
+ * feerate bucket and we increment all the counters from Y' up to max periods
+ * representing that a tx was successfullly confirmed in less than or equal to
+ * that many periods. We want to save a history of this information, so at any
+ * time we have a counter of the total number of transactions that happened in a
+ * given feerate bucket and the total number that were confirmed in each of the
+ * periods or less for any bucket.  We save this history by keeping an
+ * exponentially decaying moving average of each one of these stats.  This is
+ * done for a different decay in each of the 3 data sets to keep relevant data
+ * from different time horizons.  Furthermore we also keep track of the number
+ * unmined (in mempool or left mempool without being included in a block)
+ * transactions in each bucket and for how many blocks they have been
+ * outstanding and use both of these numbers to increase the number of transactions
+ * we've seen in that feerate bucket when calculating an estimate for any number
+ * of confirmations below the number of blocks they've been outstanding.
  */
 
+/* Identifier for each of the 3 different TxConfirmStats which will track
+ * history over different time horizons. */
 enum FeeEstimateHorizon {
     SHORT_HALFLIFE = 0,
     MED_HALFLIFE = 1,
     LONG_HALFLIFE = 2
 };
 
+/* Used to return detailed information about a feerate bucket */
 struct EstimatorBucket
 {
     double start = -1;
@@ -77,6 +84,7 @@ struct EstimatorBucket
     double leftMempool = 0;
 };
 
+/* Used to return detailed information about a fee estimate calculation */
 struct EstimationResult
 {
     EstimatorBucket pass;
@@ -93,13 +101,13 @@ struct EstimationResult
 class CBlockPolicyEstimator
 {
 private:
-    /** Track confirm delays up to 12 blocks medium decay */
+    /** Track confirm delays up to 12 blocks for short horizon */
     static constexpr unsigned int SHORT_BLOCK_PERIODS = 12;
     static constexpr unsigned int SHORT_SCALE = 1;
-    /** Track confirm delays up to 48 blocks medium decay */
+    /** Track confirm delays up to 48 blocks for medium horizon */
     static constexpr unsigned int MED_BLOCK_PERIODS = 24;
     static constexpr unsigned int MED_SCALE = 2;
-    /** Track confirm delays up to 1008 blocks for longer decay */
+    /** Track confirm delays up to 1008 blocks for long horizon */
     static constexpr unsigned int LONG_BLOCK_PERIODS = 42;
     static constexpr unsigned int LONG_SCALE = 24;
     /** Historical estimates that are older than this aren't valid */
@@ -112,9 +120,11 @@ private:
     /** Decay of .9995 is a half-life of 1008 blocks or about 1 week */
     static constexpr double LONG_DECAY = .99931;
 
-    /** Require greater than 95% of X feerate transactions to be confirmed within Y blocks for X to be big enough */
+    /** Require greater than 60% of X feerate transactions to be confirmed within Y/2 blocks*/
     static constexpr double HALF_SUCCESS_PCT = .6;
+    /** Require greater than 85% of X feerate transactions to be confirmed within Y blocks*/
     static constexpr double SUCCESS_PCT = .85;
+    /** Require greater than 95% of X feerate transactions to be confirmed within 2 * Y blocks*/
     static constexpr double DOUBLE_SUCCESS_PCT = .95;
 
     /** Require an avg of 0.1 tx in the combined feerate bucket per block to have stat significance */
@@ -154,16 +164,19 @@ public:
     /** Remove a transaction from the mempool tracking stats*/
     bool removeTx(uint256 hash, bool inBlock);
 
-    /** Return a feerate estimate */
+    /** DEPRECATED. Return a feerate estimate */
     CFeeRate estimateFee(int confTarget) const;
 
-    /** Estimate feerate needed to get be included in a block within
-     *  confTarget blocks. If no answer can be given at confTarget, return an
-     *  estimate at the lowest target where one can be given.
+    /** Estimate feerate needed to get be included in a block within confTarget
+     *  blocks. If no answer can be given at confTarget, return an estimate at
+     *  the closest target where one can be given.  'conservative' estimates are
+     *  valid over longer time horizons also.
      */
     CFeeRate estimateSmartFee(int confTarget, int *answerFoundAtTarget, const CTxMemPool& pool, bool conservative = true) const;
 
-    /** Return a specific fee estimate calculation with a given success threshold and time horizon.
+    /** Return a specific fee estimate calculation with a given success
+     * threshold and time horizon, and optionally return detailed data about
+     * calculation
      */
     CFeeRate estimateRawFee(int confTarget, double successThreshold, FeeEstimateHorizon horizon, EstimationResult *result = nullptr) const;
 
@@ -208,10 +221,15 @@ private:
     /** Process a transaction confirmed in a block*/
     bool processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry);
 
+    /** Helper for estimateSmartFee */
     double estimateCombinedFee(unsigned int confTarget, double successThreshold, bool checkShorterHorizon) const;
+    /** Helper for estimateSmartFee */
     double estimateConservativeFee(unsigned int doubleTarget) const;
+    /** Number of blocks of data recorded while fee estimates have been running */
     unsigned int BlockSpan() const;
+    /** Number of blocks of recorded fee estimate data represented in saved data file */
     unsigned int HistoricalBlockSpan() const;
+    /** Calculation of highest target that reasonable estimate can be provided for */
     unsigned int MaxUsableEstimate() const;
 };
 
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 22bff1cf9e..7fd95768a4 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -797,6 +797,7 @@ UniValue estimatefee(const JSONRPCRequest& request)
     if (request.fHelp || request.params.size() != 1)
         throw std::runtime_error(
             "estimatefee nblocks\n"
+            "\nDEPRECATED. Please use estimatesmartfee for more intelligent estimates."
             "\nEstimates the approximate fee per kilobyte needed for a transaction to begin\n"
             "confirmation within nblocks blocks. Uses virtual transaction size of transaction\n"
             "as defined in BIP 141 (witness data is discounted).\n"
@@ -831,7 +832,6 @@ UniValue estimatesmartfee(const JSONRPCRequest& request)
     if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
         throw std::runtime_error(
             "estimatesmartfee nblocks (conservative)\n"
-            "\nWARNING: This interface is unstable and may disappear or change!\n"
             "\nEstimates the approximate fee per kilobyte needed for a transaction to begin\n"
             "confirmation within nblocks blocks if possible and return the number of blocks\n"
             "for which the estimate is valid. Uses virtual transaction size as defined\n"