Fix PoS Fee Estimation Bugs

lib/block_view.go

@@ -3421,8 +3421,7 @@ func (bav *UtxoView) _connectUpdateGlobalParams(
 
 		// Validate that the minimum fee bucket size is greater than the minimum allowed.
 		mergedGlobalParams := MergeGlobalParamEntryDefaults(&newGlobalParamsEntry, bav.Params)
-		minFeeRateNanosPerKB, feeBucketMultiplier := mergedGlobalParams.
-			ComputeFeeTimeBucketMinimumFeeAndMultiplier()
+		minFeeRateNanosPerKB, feeBucketMultiplier := mergedGlobalParams.ComputeFeeTimeBucketMinimumFeeAndMultiplier()
 		nextFeeBucketMin := computeFeeTimeBucketMinFromExponent(1, minFeeRateNanosPerKB, feeBucketMultiplier)
 		if nextFeeBucketMin < mergedGlobalParams.MinimumNetworkFeeNanosPerKB+MinFeeBucketSize {
 			return 0, 0, nil, RuleErrorFeeBucketSizeTooSmall

lib/block_view_types.go

@@ -4438,15 +4438,26 @@ func (gp *GlobalParamsEntry) GetEncoderType() EncoderType {
 	return EncoderTypeGlobalParamsEntry
 }
 
-// ComputeFeeTimeBucketMinimumFeeAndMultiplier takes the MinimumNetworkFeeNanosPerKB and FeeBucketGrowthRateBasisPoints for
-// the GlobalParamsEntry, and returns them as big.Floats.
+// ComputeFeeTimeBucketMinimumFeeAndMultiplier takes the MinimumNetworkFeeNanosPerKB and FeeBucketGrowthRateBasisPoints,
+// scales the growth rate into a multiplier, and returns the result as big.Floats.
 func (gp *GlobalParamsEntry) ComputeFeeTimeBucketMinimumFeeAndMultiplier() (
-	_minimumRate *big.Float, _bucketMultiplier *big.Float) {
-
+	_minimumRate *big.Float,
+	_bucketMultiplier *big.Float,
+) {
+	minimumNetworkFeeNanosPerKB, growthRateBasisPoints := gp.GetFeeTimeBucketMinimumFeeAndGrowthRateBasisPoints()
+	return minimumNetworkFeeNanosPerKB, ComputeMultiplierFromGrowthRateBasisPoints(growthRateBasisPoints)
+}
+
+// GetFeeTimeBucketMinimumFeeAndGrowthRateBasisPoints returns the the MinimumNetworkFeeNanosPerKB and
+// FeeBucketGrowthRateBasisPoints params as returns them as big.Floats.
+func (gp *GlobalParamsEntry) GetFeeTimeBucketMinimumFeeAndGrowthRateBasisPoints() (
+	_minimumRate *big.Float,
+	_bucketMultiplier *big.Float,
+) {
 	minimumNetworkFeeNanosPerKB := NewFloat().SetUint64(gp.MinimumNetworkFeeNanosPerKB)
-	feeBucketMultiplier := NewFloat().SetUint64(10000 + gp.FeeBucketGrowthRateBasisPoints)
-	feeBucketMultiplier.Quo(feeBucketMultiplier, NewFloat().SetUint64(10000))
-	return minimumNetworkFeeNanosPerKB, feeBucketMultiplier
+	growthRateBasisPoints := NewFloat().SetUint64(gp.FeeBucketGrowthRateBasisPoints)
+
+	return minimumNetworkFeeNanosPerKB, growthRateBasisPoints
 }
 
 // This struct holds info on a readers interactions (e.g. likes) with a post.

lib/blockchain.go

@@ -5010,9 +5010,15 @@ func (bc *Blockchain) CreateMaxSpend(
 				if bc.params.IsPoSBlockHeight(uint64(bc.BlockTip().Height)) {
 					maxBlockSizeBytes = utxoView.GetSoftMaxBlockSizeBytesPoS()
 				}
-				// TODO: replace MaxBasisPoints with variables configured by flags.
-				feeAmountNanos, err = mempool.EstimateFee(txn, minFeeRateNanosPerKB,
-					MaxBasisPoints, MaxBasisPoints, MaxBasisPoints, MaxBasisPoints, maxBlockSizeBytes)
+				feeAmountNanos, err = mempool.EstimateFee(
+					txn,
+					minFeeRateNanosPerKB,
+					// TODO: Make these flags or GlobalParams
+					bc.params.MempoolCongestionFactorBasisPoints,
+					bc.params.MempoolPriorityPercentileBasisPoints,
+					bc.params.PastBlocksCongestionFactorBasisPoints,
+					bc.params.PastBlocksPriorityPercentileBasisPoints,
+					maxBlockSizeBytes)
 				if err != nil {
 					return nil, 0, 0, 0, errors.Wrapf(err, "CreateMaxSpend: Problem estimating fee: ")
 				}
@@ -5147,9 +5153,15 @@ func (bc *Blockchain) AddInputsAndChangeToTransactionWithSubsidy(
 				if bc.params.IsPoSBlockHeight(uint64(bc.BlockTip().Height)) {
 					maxBlockSizeBytes = utxoView.GetSoftMaxBlockSizeBytesPoS()
 				}
-				// TODO: replace MaxBasisPoints with variables configured by flags.
-				newTxFee, err := mempool.EstimateFee(txArg, minFeeRateNanosPerKB, MaxBasisPoints,
-					MaxBasisPoints, MaxBasisPoints, MaxBasisPoints, maxBlockSizeBytes)
+				newTxFee, err := mempool.EstimateFee(
+					txArg,
+					minFeeRateNanosPerKB,
+					// TODO: Make these flags or GlobalParams
+					bc.params.MempoolCongestionFactorBasisPoints,
+					bc.params.MempoolPriorityPercentileBasisPoints,
+					bc.params.PastBlocksCongestionFactorBasisPoints,
+					bc.params.PastBlocksPriorityPercentileBasisPoints,
+					maxBlockSizeBytes)
 				UpdateTxnFee(txArg, newTxFee)
 				if err != nil {
 					return 0, 0, 0, 0, errors.Wrapf(err,
@@ -5863,7 +5875,15 @@ func (bc *Blockchain) CreateAtomicTxnsWrapper(
 		txn.ExtraData[NextAtomicTxnPreHash] = dummyAtomicHashBytes
 		txn.ExtraData[PreviousAtomicTxnPreHash] = dummyAtomicHashBytes
 		newFeeEstimate, err := mempool.EstimateFee(
-			txn, 0, MaxBasisPoints, MaxBasisPoints, MaxBasisPoints, MaxBasisPoints, maxBlockSizeBytes)
+			txn,
+			// TODO: Allow the caller to specify minFeeRateNanosPerKB
+			0,
+			// TODO: Make these flags or GlobalParams
+			bc.params.MempoolCongestionFactorBasisPoints,
+			bc.params.MempoolPriorityPercentileBasisPoints,
+			bc.params.PastBlocksCongestionFactorBasisPoints,
+			bc.params.PastBlocksPriorityPercentileBasisPoints,
+			maxBlockSizeBytes)
 		if err != nil {
 			return nil, 0, errors.Wrapf(err, "CreateAtomicTxnsWrapper: failed to recompute fee estimate")
 		}
@@ -5949,7 +5969,14 @@ func (bc *Blockchain) CreateAtomicTxnsWrapper(
 		// Use EstimateFee to set the fee INCLUDING the wrapper. Note that this fee should generally be a bit
 		// higher than the totalFee computed above because the atomic wrapper adds overhead.
 		newFeeEstimate, err := mempool.EstimateFee(
-			atomicTxn, 0, MaxBasisPoints, MaxBasisPoints, MaxBasisPoints, MaxBasisPoints, maxBlockSizeBytes)
+			atomicTxn,
+			0,
+			// TODO: Make these flags or GlobalParams
+			bc.params.MempoolCongestionFactorBasisPoints,
+			bc.params.MempoolPriorityPercentileBasisPoints,
+			bc.params.PastBlocksCongestionFactorBasisPoints,
+			bc.params.PastBlocksPriorityPercentileBasisPoints,
+			maxBlockSizeBytes)
 		if err != nil {
 			return nil, 0, errors.Wrapf(err, "CreateAtomicTxnsWrapper: failed to compute "+
 				"fee on full txn")

lib/constants.go

@@ -785,12 +785,13 @@ type DeSoParams struct {
 
 	// DefaultMempoolFeeEstimatorNumMempoolBlocks is the default value for
 	// GlobalParamsEntry.MempoolFeeEstimatorNumMempoolBlocks. See the comment in GlobalParamsEntry
-	// for a description of its usage.
+	// for a description of its usage. Also see the comment on the setting in DeSoMainnetParams
 	DefaultMempoolFeeEstimatorNumMempoolBlocks uint64
 
 	// DefaultMempoolFeeEstimatorNumPastBlocks is the default value for
 	// GlobalParamsEntry.MempoolFeeEstimatorNumPastBlocks. See the comment in GlobalParamsEntry
-	// for a description of its usage.
+	// for a description of its usage. Also see the comment on the DeSoMainnetParams value of
+	// this setting.
 	DefaultMempoolFeeEstimatorNumPastBlocks uint64
 
 	// DefaultMaxBlockSizeBytesPoS is the default value for GlobalParamsEntry.MaxBlockSizeBytesPoS.
@@ -822,6 +823,12 @@ type DeSoParams struct {
 
 	ForkHeights ForkHeights
 
+	// See comment on the DeSoMainnetParams settings of these values
+	MempoolCongestionFactorBasisPoints      uint64
+	MempoolPriorityPercentileBasisPoints    uint64
+	PastBlocksCongestionFactorBasisPoints   uint64
+	PastBlocksPriorityPercentileBasisPoints uint64
+
 	EncoderMigrationHeights     *EncoderMigrationHeights
 	EncoderMigrationHeightsList []*MigrationHeight
 }
@@ -1288,10 +1295,39 @@ var DeSoMainnetParams = DeSoParams{
 	// The maximum size of the mempool in bytes.
 	DefaultMempoolMaxSizeBytes: 3 * 1024 * 1024 * 1024, // 3GB
 
-	// The number of future blocks to consider when estimating the mempool fee.
+	// The number of future blocks to consider when estimating the mempool fee. Setting this
+	// value to 1 means we will start to increase fees if the mempool has 1 block's worth of
+	// txns in it, and decrease them if it has less. Note that a setting of 1 is somewhat
+	// aggresive, but it's good because it ensures that the typical fee estimate we give will
+	// be highly likely to get one's transaction included in the next block.
+	//
+	// Note that if you are *blasting* txns at the mempool, then having this value set to 1 may
+	// cause the fee estimator to report a higher and higher fee as you're constructing and
+	// submitting txns (assuming you are sending txns faster than they are going into blocks).
+	// This can cause txns that you submit later to have higher fees, which will cause them to
+	// sort to the *front* of the mempool, potentially causing dependency issues for you. If you
+	// absolutely need txns to run in a specific order, you have several options:
+	//
+	// 1. Query the fee estimator for the fee you should use for your txns *before* you construct
+	//    them, and then construct your txns by explicitly specifying that fee. As long as you use
+	//    the same fee for all of your txns, and as long as you submit them directly to the current leader,
+	//    you should be guaranteed to have them go into the next blocks in order. This is because the
+	//    mempool uses a smart fee bucketing approach, whereby txns that pay similar fees are ordered
+	//    by time (within a fee bucket). Alternatively, you can just set a fee above the minimum
+	//    manually, which will get your txn included in the blocks eventually. At the time of this
+	//    writing, a fee of 1,000 nanos per kb was well above what was needed to get into the next
+	//    block but still quite cheap (1/10,000th of a cent).
+	//
+	// 2. Use an atomic txn to submit all of your txns at once. This will ensure that they either
+	//    all go through or all fail together.
+	//
+	// 3. Slow down your txn submission to ensure that txns are going into blocks before
+	//    their dependencies are submitted.
 	DefaultMempoolFeeEstimatorNumMempoolBlocks: 1,
 
-	// The number of past blocks to consider when estimating the mempool fee.
+	// The number of past blocks to consider when estimating the mempool fee. This is
+	// means that we will increase or decrease fees based on the past minute's worth of
+	// blocks dynamically.
 	DefaultMempoolFeeEstimatorNumPastBlocks: 50,
 
 	// The maximum size of blocks for PoS.
@@ -1315,6 +1351,30 @@ var DeSoMainnetParams = DeSoParams{
 	// DisableNetworkManagerRoutines is a testing flag that disables the network manager routines.
 	DisableNetworkManagerRoutines: false,
 
+	// The congestion factor determines when we will start to increase or decrease fees.
+	// We set the congestion factor to 90% for past blocks and mempool. This makes it so that we will
+	// start to increase fees when the past N blocks (DefaultMempoolFeeEstimatorNumPastBlocks) are
+	// 90% full on average or the mempool has 90% of 1 block's worth of txns in it (actually 90% of
+	// DefaultMempoolFeeEstimatorNumMempoolBlocks). This is good because it ensures that the typical
+	// fee estimate we give will be highly likely to get one's transaction included in the next block
+	// or, at worst, a block within about a minute (for N=50).
+	//
+	// Using the 90th percentile allows the fee market to be aggressive, but it's better than using
+	// 100% because that can have some rounding issues. For example, if you use 100% and blocks are
+	// 99% full, the fee market won't adapt. So it's better to have a little slack.
+	MempoolCongestionFactorBasisPoints:    uint64(9000),
+	PastBlocksCongestionFactorBasisPoints: uint64(9000),
+	// The priority percentile determines what benchmark we use to increase the fee we're paying. For
+	// past blocks, we set a percentile of 90%, which means we'll take the fee paid by the 90th percentile
+	// txn in the past N blocks and increase it by one fee bucket. This works nicely with N=50 blocks
+	// because the 90th percentile will be within 5 blocks if you sorted all txns by their fees. For the
+	// mempool, we set a percentile of 10%, which means we use the fee paid by the 10th percentile txn in
+	// the highest 1 block's worth of txns in the mempool. We use a lower percentile here because the mempool
+	// has a much tighter window of a single block, and so by outbidding *anybody* in that block, you're
+	// already highly likely to get in.
+	MempoolPriorityPercentileBasisPoints:    uint64(1000),
+	PastBlocksPriorityPercentileBasisPoints: uint64(9000),
+
 	ForkHeights:                 MainnetForkHeights,
 	EncoderMigrationHeights:     GetEncoderMigrationHeights(&MainnetForkHeights),
 	EncoderMigrationHeightsList: GetEncoderMigrationHeightsList(&MainnetForkHeights),
@@ -1623,6 +1683,12 @@ var DeSoTestnetParams = DeSoParams{
 	// DisableNetworkManagerRoutines is a testing flag that disables the network manager routines.
 	DisableNetworkManagerRoutines: false,
 
+	// See comment on DeSoMainnetParams
+	MempoolCongestionFactorBasisPoints:      uint64(9000),
+	PastBlocksCongestionFactorBasisPoints:   uint64(9000),
+	MempoolPriorityPercentileBasisPoints:    uint64(1000),
+	PastBlocksPriorityPercentileBasisPoints: uint64(9000),
+
 	ForkHeights:                 TestnetForkHeights,
 	EncoderMigrationHeights:     GetEncoderMigrationHeights(&TestnetForkHeights),
 	EncoderMigrationHeightsList: GetEncoderMigrationHeightsList(&TestnetForkHeights),

lib/pos_fee_estimator.go

@@ -667,26 +667,32 @@ func (posFeeEstimator *PoSFeeEstimator) estimateFeeRateNanosPerKBGivenTransactio
 		txnRegister.minimumNetworkFeeNanosPerKB,
 		txnRegister.feeBucketGrowthRateBasisPoints,
 	)
-	// If the bucketMinFee is less than or equal to the global min fee rate, we return the global min fee rate.
-	if bucketMinFee <= globalMinFeeRate {
-		return globalMinFeeRate
-	}
 
 	// Compute the congestion threshold. If our congestion factor is 100% (or 10,000 bps),
 	// then congestion threshold is simply max block size * numPastBlocks
-	// TODO: I don't know if I like this name really.
 	congestionThreshold := (congestionFactorBasisPoints * maxSizeOfNumBlocks) / MaxBasisPoints
 	// If the total size of the txns in the transaction register is less than the computed congestion threshold,
 	// we return one bucket lower than the Priority fee.
 	if totalTxnsSize <= congestionThreshold {
+		// When we're below the congestion threshold, we want to suggest one fee bucket *lower*
+		// than the Priority fee we got in the previous step. This mechanism allows fees to drop
+		// dynamically during times of low congestion.
+		if bucketMinFee <= globalMinFeeRate {
+			// If the Priority fee we got from the previous step is <= the global min, then we *can't* suggest
+			// a lower fee, so just return the global min.
+			return globalMinFeeRate
+		}
 		// Return one bucket lower than Priority fee
+		feeBucketMultiplier := ComputeMultiplierFromGrowthRateBasisPoints(txnRegister.feeBucketGrowthRateBasisPoints)
 		bucketExponent := computeFeeTimeBucketExponentFromFeeNanosPerKB(
-			bucketMinFee, txnRegister.minimumNetworkFeeNanosPerKB, txnRegister.feeBucketGrowthRateBasisPoints)
+			bucketMinFee, txnRegister.minimumNetworkFeeNanosPerKB, feeBucketMultiplier)
 		return computeFeeTimeBucketMinFromExponent(
-			bucketExponent-1, txnRegister.minimumNetworkFeeNanosPerKB, txnRegister.feeBucketGrowthRateBasisPoints)
+			bucketExponent-1, txnRegister.minimumNetworkFeeNanosPerKB, feeBucketMultiplier)
 	}
 
-	// Otherwise, we return one bucket higher than Priority fee
+	// Otherwise, if we're above the congestion threshold, we return one bucket higher than
+	// the Priority fee. This mechanism allows fees to rise dynamically during times of high
+	// congestion.
 	return bucketMaxFee + 1
 }
 

lib/pos_mempool.go

@@ -259,6 +259,9 @@ func (mp *PosMempool) Init(
 	maxValidationViewConnects uint64,
 	transactionValidationRefreshIntervalMillis uint64,
 ) error {
+	mp.Lock()
+	defer mp.Unlock()
+
 	if mp.status != PosMempoolStatusNotInitialized {
 		return errors.New("PosMempool.Init: PosMempool already initialized")
 	}
@@ -282,6 +285,12 @@ func (mp *PosMempool) Init(
 	mp.recentBlockTxnCache = lru.NewKVCache(100000)    // cache 100K latest txns from blocks.
 	mp.recentRejectedTxnCache = lru.NewKVCache(100000) // cache 100K rejected txns.
 
+	// Recreate and initialize the transaction register and the nonce tracker.
+	mp.txnRegister = NewTransactionRegister()
+	mp.txnRegister.Init(mp.globalParams)
+	mp.nonceTracker = NewNonceTracker()
+
+	// Initialize the fee estimator
 	err = mp.feeEstimator.Init(mp.txnRegister, feeEstimatorPastBlocks, mp.globalParams)
 	if err != nil {
 		return errors.Wrapf(err, "PosMempool.Start: Problem initializing fee estimator")
@@ -298,11 +307,6 @@ func (mp *PosMempool) Start() error {
 		return errors.New("PosMempool.Start: PosMempool not initialized")
 	}
 
-	// Create the transaction register, the ledger, and the nonce tracker,
-	mp.txnRegister = NewTransactionRegister()
-	mp.txnRegister.Init(mp.globalParams)
-	mp.nonceTracker = NewNonceTracker()
-
 	// Setup the database and create the persister
 	if !mp.inMemoryOnly {
 		mempoolDirectory := filepath.Join(mp.dir, "mempool")
@@ -321,11 +325,13 @@ func (mp *PosMempool) Start() error {
 			return errors.Wrapf(err, "PosMempool.Start: Problem loading persisted transactions")
 		}
 	}
+
 	mp.startGroup.Add(1)
 	mp.exitGroup.Add(1)
 	mp.startTransactionValidationRoutine()
 	mp.startGroup.Wait()
 	mp.status = PosMempoolStatusRunning
+
 	return nil
 }