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

Feat: add heuristics to miner for nakamoto #5238

Merged
merged 22 commits into from
Oct 5, 2024
Merged

Conversation

kantai
Copy link
Member

@kantai kantai commented Sep 24, 2024

This add two heuristics for mining in nakamoto:

  • For the first block in a tenure, just mine an empty block
  • Estimate the time it takes to eval a tx, and see if it will interfere with block deadline. If it will, skip the transaction.

Marking as draft until tests can be fleshed out and likely broken integration tests fixed.

* for the first block in a tenure, just mine an empty block
* estimate the time it takes to eval a tx, and see if it will interfere with block deadline
@kantai kantai requested a review from a team as a code owner September 24, 2024 22:13
@kantai kantai marked this pull request as draft September 24, 2024 22:14
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.

I think the idea is sound, but testing this in a reproducible way is gonna be interesting.

Copy link
Contributor

@obycode obycode 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 like this addition! Just noticed one rogue comment.

obycode
obycode previously approved these changes Sep 25, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

👍

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.

The changes look fine to me, but I'm curious if the mempool changes are sufficient to stave off a network stall in fuzzing. The code will only detect that a transaction would exceed its designated processing time after it has been run once. But if the fuzzer is bombarding the network with lots and lots of expensive transactions, which are only meant to ever be evaluated once, then this wouldn't materially change anything.

Do we know for sure that the cause of block production slowness in fuzzing is due to re-considering the same transaction over and over?

Also, would this change mean that some transactions are never mined, despite being valid, simply because they were considered once and effectively blocked from re-consideration due to taking too long to run? When it comes time to rolling this out into production, we'll want to be sure that this doesn't lead to some contracts being effectively blocked from executing.

@kantai
Copy link
Member Author

kantai commented Sep 30, 2024

The changes look fine to me, but I'm curious if the mempool changes are sufficient to stave off a network stall in fuzzing. The code will only detect that a transaction would exceed its designated processing time after it has been run once. But if the fuzzer is bombarding the network with lots and lots of expensive transactions, which are only meant to ever be evaluated once, then this wouldn't materially change anything.

That's mostly true, but its also an argument against incremental improvement. Does this change address all possible long running transactions in the mempool? No, but it does at least prevent the same long running transaction from repeatedly preventing the miner from making progress.

Do we know for sure that the cause of block production slowness in fuzzing is due to re-considering the same transaction over and over?

In the particular case that this addresses, yes.

Also, would this change mean that some transactions are never mined, despite being valid, simply because they were considered once and effectively blocked from re-consideration due to taking too long to run? When it comes time to rolling this out into production, we'll want to be sure that this doesn't lead to some contracts being effectively blocked from executing.

That would be ultimately up to the miner and signer sets. Having an estimate for how long a transaction will take to run and comparing against the miners self-configured execution deadline doesn't seem like a policy change vis-a-vis long-running transactions: rather, it just makes the miner somewhat smarter about hitting its deadling.

@jcnelson
Copy link
Member

That's mostly true, but its also an argument against incremental improvement. Does this change address all possible long running transactions in the mempool? No, but it does at least prevent the same long running transaction from repeatedly preventing the miner from making progress.

Apologies if I sounded like I was against incremental improvement. I was only trying to confirm my own understanding.

@kantai kantai marked this pull request as ready for review October 4, 2024 17:32
@kantai kantai requested a review from a team as a code owner October 4, 2024 17:32
@kantai
Copy link
Member Author

kantai commented Oct 4, 2024

New tests and assertions added for the behaviors, and all existing tests pass. Ready for review!

Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

Lgtm!

@kantai kantai added this pull request to the merge queue Oct 5, 2024
Merged via the queue into develop with commit 3261d0d Oct 5, 2024
1 check passed
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants