stacks-network / stacks-core

The Stacks blockchain implementation
https://docs.stacks.co
GNU General Public License v3.0
3.01k stars 671 forks source link

Feat: add heuristics to miner for nakamoto #5238

Closed kantai closed 1 month ago

kantai commented 1 month ago

This add two heuristics for mining in nakamoto:

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

kantai commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

blockstack-devops commented 3 weeks ago

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.