-
Notifications
You must be signed in to change notification settings - Fork 20.2k
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
miner: avoid sleeping in miner #22108
Conversation
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.
lookin' good
Hmm, this check was introduced in a8ebf75#diff-689874b30f6f905ce6c47cdefeddcf052b467a7936a981f4b6cf38939e10d924R378. Doesn't seem to be the hackathon thing I remembered. Are we sure there's no other catch? |
What happens if I mine multiple subsequent blocks with the same timestamp? I guess Clique does that in dev mode and it's fine? Edit: Seems ethash prohibits same timestamped blocks, but Clique with 0 period explicitly allows it. |
You should not ever do that in ethash, since the child block has at least |
It's same ( + 1s ) for clique. |
My concern is that if we remove this clause, then we run the risk of mining blocks with the same timestamp. |
Why? The timestamp is always bumped at least 1 second even if it's the future block |
Out of curiosity, tried disabling this clause and running dev mode with ethash. It's mining multiple blocks per second, but each has a timestamp of parent+1. What happens if I mine 30+ blocks, exceeding the future allowance of chain importing? The miner is a bit special in that it inserts blocks directly into the database (AFAIK). Could it happen that my local miner sees one chain, but the entire network rejects that as being in the future. Just thinking out aloud here. |
re-posting from discord: If you mine 16s into the future, your peers will ignore it for a second, but after that they'll accept it. And it does make sense to keep building the next one, if you have nothing better to do. What should happen, if you go too far into the future, is that your blocks become orphaned, and you become "kicked back" to present by importing a canonical block. My feeling is that the miner shouldn't need to care about the 15s rule -- seems like that should happen organically |
However, in dev-mode, with disabled PoW checks, this problem arises. And I think it's exactly what the original code tries to fix. But I don't think it's a fix we need anymore, since devmode now uses clique by default. |
This PR removes a logic in the miner, which was originally intended to help temporary testnets based on ethash from "running off into the future". If the difficulty was low, and a few computers started mining several blocks per second, the ethash rules (which demand 1s delay between blocks) would push the blocktimes further and further away.
The solution was to make the miner sleep while this happened.
Nowadays, this problem is solved instead by PoA chains, and it's recommended to let testnets and devnets be based on clique instead. The existing logic is problematic, since it can cause stalls within the miner making it difficult for remote workers to submit work if the channel is blocked on a sleep.