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

Bit boundary fix for the DAG generation routine #223

Merged
merged 2 commits into from Nov 6, 2020
Merged

Bit boundary fix for the DAG generation routine #223

merged 2 commits into from Nov 6, 2020

Conversation

slavikus
Copy link
Contributor

@slavikus slavikus commented Nov 6, 2020

If DAG size is exceeding the maximum 32 bit unsigned value, the DAG generation will produce incorrect results depending on the number of threads available on the host machine. Switch the generation to 64 bit variable to fix this.

Note that a hard fork may be required on the ETC network for the time being as we're already on the epoch 385 which generates a DAG file of 4.008 Gb, causing some nodes to reject valid blocks, depending on the conditions under which they have generated a new DAG file under.

@meowsbits
Copy link
Member

Thanks for this. We are running a few sync tests and general otherwise sanity checks, but in general this looks good and unless we find something unexpected we'll have it merged very soon.

@meowsbits
Copy link
Member

I just completed a successful test where I

  • rm -rfd the Ethash DAG datadir and cachedir
  • debug.setHead to block 11m (which was past my previous fast sync pivot, thus enabling fast sync mode)
  • resynced --classic from there

Node completed sync successfully.

I have also run some basic checks on the interoperability of the ethash cache (etchash) data between the current master version and this PR for a non-mining node to show at least that things won't explode when users upgrade.

It will probably be safest for miners to regenerate their DAG dirs and caches when they upgrade.

Copy link
Member

@meowsbits meowsbits left a comment

Choose a reason for hiding this comment

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

@meowsbits
Copy link
Member

@iquidus, please give your review and merge if and when OK by you too. We'll aim for tagging and publishing a release today.

iquidus
iquidus approved these changes Nov 6, 2020
@iquidus iquidus merged commit 98f92db into etclabscore:master Nov 6, 2020
1 of 2 checks passed
@bobsummerwill
Copy link
Member

@meowsbits "It will probably be safest for miners to regenerate their DAG dirs and caches when they upgrade."

With the https://github.com/etclabscore/core-geth/releases/tag/v1.11.17 release today, would that be your only recommendation?

For all Core-Geth users to upgrade and further for miners to regenerate?

And it does not appear that any blocks have been mined with broken DAGs, eh? We are not dependent on replicating the bug into the future, right?

@meowsbits
Copy link
Member

Yes, yes, yes, yes! 🥇

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.

None yet

5 participants