-
Notifications
You must be signed in to change notification settings - Fork 668
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
Conversation
* 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
There was a problem hiding this 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.
There was a problem hiding this 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.
…tx in the first block after extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this 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.
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.
In the particular case that this addresses, yes.
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. |
Apologies if I sounded like I was against incremental improvement. I was only trying to confirm my own understanding. |
* ci: disable unneeded microblocks tests * test: remove blanket 2 sec wait for nakamoto btc blocks
New tests and assertions added for the behaviors, and all existing tests pass. Ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!
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. |
This add two heuristics for mining in nakamoto:
Marking as draft until tests can be fleshed out and likely broken integration tests fixed.