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

Fix PoS mempool dynamic fee market #1252

Conversation

diamondhands0
Copy link
Member

@diamondhands0 diamondhands0 commented Apr 20, 2024

A bunch of things were broken in our mempool fee estimation logic. All fixed now. Summary:

  • Fixed a bug whereby we would never increase the fee above the minimum fee because of the placement of a if bucketMinFee <= globalMinFeeRate check.
  • Fixed a bug where we were overwriting the mempool fee register in Start(), which was causing the mempool fee estimator to have no txns in it, and thus always return the minimum value. This meant we were not considering mempool congestion at all.
  • In many places, we were confusing "fee bucket growth rate basis points" with "fee bucket multiplier". The former value would be something like 1000 (= 10%) while the latter would map to 1.1 (= 10000 + 1000 / 10000). This caused fee-time ordering to be basically completely broken. All fixed now, and fixed tests.
    • Just to add a little more detail: The tests were very well-written and I think they exercise this logic very well. The reason why they were passing before, though, is because we were setting the value incorrectly in the Init() and passed it wrong as an argument, and the two sortof compensated for each other in the tests. But in production, we would go down a different path that wouldn't compensate properly, which is how I found the bug. Anyway it's all fixed now.
  • Set optimized defaults for the mempool dynamic fee params and added a deep comment explaining why we chose these values where they are define. Also made sure we're using them consistently in all the relevant places. These params optimize heavily toward getting your txn into the next block, which is what we want. They cause reordering issues if you're sending txns at a rate much higher than 1 block per second, but this is correct behavior, and the comments include suggestions on how to mitigate these issues (eg by manually setting the fee or using an atomic txn):
    • MempoolCongestionFactorBasisPoints
    • MempoolPriorityPercentileBasisPoints
    • PastBlocksCongestionFactorBasisPoints
    • PastBlocksPriorityPercentileBasisPoints
  • In computeFeeTimeBucketRangeFromExponent, there was a weird edge-case where we could have a fee bucket with start less than end. This can't happen in a real scenario, though, only when the bucket growth rate is like 1bp, which is ridiculously small. And I only found it because of the growth rate <> multiplier issue mentioned previously, which was causing a 10% growth rate to be threaded through as 1bp.

For reference, in case it's useful, the way I found all this stuff was I slowed the block time down to 1 block every 10s and made the NumPastBlocks for the block estimator 5 blocks using the params in constants.go (so that txns would accumulate in the mempool) and added logging of the fees. Then I wrote a script that blasted the mempool with txns and noticed that the fees weren't adjusting properly, which led me down the rabbit-hole to find all of these issues. After fixing all the issues I took some time to optimize all the params, and then used my script to exercise everything and make sure it's fully 100% adapting correctly. Specifically, I saw that the fee goes up correctly once the mempool has a full block's worth of txns accumulated in it, stays high for a few blocks because of the block estimator, and then starts to go down as more blocks come through. It all works really well.

@diamondhands0 diamondhands0 requested a review from a team as a code owner April 20, 2024 02:50
@diamondhands0 diamondhands0 changed the title TODO: title and description Fix PoS mempool dynamic fee market Apr 20, 2024
lib/blockchain.go Show resolved Hide resolved
txn, 0, MaxBasisPoints, MaxBasisPoints, MaxBasisPoints, MaxBasisPoints, maxBlockSizeBytes)
txn,
// TODO: Allow the caller to specify minFeeRateNanosPerKB
0,
Copy link
Member

Choose a reason for hiding this comment

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

I can update this in a separate PR. It's a relatively minor change.

lib/pos_fee_estimator.go Outdated Show resolved Hide resolved
@tholonious tholonious changed the base branch from feature/proof-of-stake to Mempool_Minor_Naming_and_Testing_Cleanup April 22, 2024 17:16
@tholonious tholonious force-pushed the diamondhands/KILL-LAST-MEMPOOL-FEE-ESTIMATION-BUGS branch from 3710328 to a14ce53 Compare April 22, 2024 17:16
Copy link
Contributor

tholonious commented Apr 22, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @diamondhands0 and the rest of your teammates on Graphite Graphite

Base automatically changed from Mempool_Minor_Naming_and_Testing_Cleanup to feature/proof-of-stake April 22, 2024 18:18
@tholonious tholonious force-pushed the diamondhands/KILL-LAST-MEMPOOL-FEE-ESTIMATION-BUGS branch from 343fa7f to 6a5aca6 Compare April 22, 2024 18:31
@tholonious tholonious self-requested a review April 22, 2024 18:36
Copy link
Contributor

@tholonious tholonious left a comment

Choose a reason for hiding this comment

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

LGTM. I moved the overrideFeeRateNanosPerKB change to #1255. That way, we can merge this rest of this PR asap.

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.

3 participants