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: update miner mempool iterator query to consider both nonces and fee rates #5541

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

rafaelcr
Copy link

@rafaelcr rafaelcr commented Dec 7, 2024

Problem

The current query used by Stacks miners to iterate over pending mempool transactions sorts them only by fee_rate in descending order. This approach creates the following problems:

  1. Since it does not consider transaction nonces, non-mineable transactions with high fees but old (or very future) nonces go to the top of the list and get visited every time, thus wasting block construction time budget.
  2. Transactions with null fee rates are sent to the bottom of the list even if their nonces should be evaluated next, forcing the algorithm to query them separately with some probability of inclusion.

Solution

This PR adds a new mempool walk strategy (NextNonceWithHighestFeeRate) which prioritizes transactions that can be confirmed as fast as possible (next expected nonces for both origin and sponsor) with the highest fee rates as possible. This means that even if it doesn't select the transactions with the highest global fees first, it will select those that can be mined immediately therefore optimizing block building time and allowing the miner to fit in more transactions in total within the allotted time. This strategy also mixes in transactions with null fee rates in results by simulating a fee rate for the purposes of ordering only.

For easier testing, a mempool walk strategy enum value was added to the MinerConfig, which includes:

  • GlobalFeeRate: Current global fee rate sorting behavior. Enabled by default.
  • NextNonceWithHighestFeeRate: The new strategy added by this PR

@CLAassistant
Copy link

CLAassistant commented Dec 7, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rafaelcr rafaelcr changed the title (draft) fix: update miner mempool transaction iterator query to consider fee rate and nonce fix: update miner mempool transaction iterator query to consider fee rate and nonce Dec 15, 2024
@rafaelcr rafaelcr marked this pull request as ready for review December 15, 2024 00:09
@rafaelcr rafaelcr requested a review from a team as a code owner December 15, 2024 00:09
@rafaelcr rafaelcr changed the title fix: update miner mempool transaction iterator query to consider fee rate and nonce fix: update miner mempool iterator query to consider both nonces and fee rates Dec 15, 2024
@obycode obycode added this to the 3.1.0.0.3 milestone Dec 16, 2024
@obycode obycode self-requested a review December 16, 2024 20:32
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @rafaelcr!

This is something that I think is going to need somewhat extensive real-world testing before we make it the default behavior. Do you think you could add this new mempool walk logic as an opt-in alternative, which the node operator can opt to use? This would give miners a chance to compare/contrast the new behavior with the old behavior in an easy-to-rollback manner. A subsequent PR could make the new behavior the default.

What do you think?

@rafaelcr
Copy link
Author

Great idea @jcnelson and thanks for the feedback! I'll get on it.

@jcnelson
Copy link
Member

jcnelson commented Dec 17, 2024

From the call:

  • We need @jbencin's try_mine PR so we can verify that this improves the time to mine blocks.
  • Is the nonce cache preserved between blocks in Nakamoto? Not caching this will have a high regressive performance impact on Nakamoto that wasn't there in Stacks 2.x.

@jbencin
Copy link
Contributor

jbencin commented Dec 17, 2024

@jcnelson @rafaelcr I have a PR that adds Nakamoto support to stacks-inspect try-mine (#5584), feel free to try it out. For benchmarking, I'd suggest building both versions as release binaries:

cargo build --release --bin=stacks-inspect

And using hyperfine (you can cargo install it) to compare the two. Example:

hyperfine -w 3 -r 20 "command 1" "command 2"

@rafaelcr
Copy link
Author

@obycode @jcnelson I just updated the code to

  1. Create the new mempool walk strategy, disabled by default, configurable via miner config
  2. Consider sponsor nonces as well in selection query
  3. Expand unit test to mix in sponsored and not sponsored transactions with fee variety and cross-sponsor variety

Can you take another look? Local tests look very good, including the query plan which uses only indexes

Also looking forward to trying this out with @jbencin 's tools once you think it's in good shape

@rafaelcr rafaelcr requested review from obycode and jcnelson December 18, 2024 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: In Review
Development

Successfully merging this pull request may close these issues.

5 participants