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

miner: avoid sleeping in miner #22108

Merged
merged 1 commit into from
Jan 5, 2021
Merged

miner: avoid sleeping in miner #22108

merged 1 commit into from
Jan 5, 2021

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Jan 4, 2021

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.

Copy link

@DGKSK8LIFE DGKSK8LIFE left a comment

Choose a reason for hiding this comment

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

lookin' good

@holiman holiman added this to the 1.10.0 milestone Jan 4, 2021
@karalabe
Copy link
Member

karalabe commented Jan 5, 2021

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?

@karalabe
Copy link
Member

karalabe commented Jan 5, 2021

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.

@holiman
Copy link
Contributor Author

holiman commented Jan 5, 2021

What happens if I mine multiple subsequent blocks with the same timestamp?

You should not ever do that in ethash, since the child block has at least +1 from parent. For clique, I don't know.

@rjl493456442
Copy link
Member

rjl493456442 commented Jan 5, 2021

For clique, I don't know.

It's same ( + 1s ) for clique.

@karalabe
Copy link
Member

karalabe commented Jan 5, 2021

My concern is that if we remove this clause, then we run the risk of mining blocks with the same timestamp.

@rjl493456442
Copy link
Member

Why? The timestamp is always bumped at least 1 second even if it's the future block

@karalabe
Copy link
Member

karalabe commented Jan 5, 2021

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.

@holiman
Copy link
Contributor Author

holiman commented Jan 5, 2021

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

@holiman
Copy link
Contributor Author

holiman commented Jan 5, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants