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

[ETCM-109] ECIP-1099 implementation #764

Merged
merged 6 commits into from
Oct 30, 2020
Merged

[ETCM-109] ECIP-1099 implementation #764

merged 6 commits into from
Oct 30, 2020

Conversation

mmrozek
Copy link
Contributor

@mmrozek mmrozek commented Oct 29, 2020

Copy link
Contributor

@kapke kapke left a comment

Choose a reason for hiding this comment

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

Overall looks good to me!

val epoch = EthashUtils.epoch(parentBlock.header.number.toLong + 1)

val blockNumber = parentBlock.header.number.toLong + 1
val epoch = EthashUtils.epoch(blockNumber, blockCreator.blockchainConfig.ecip1099BlockNumber.toLong)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not accepting BigInt there? I mean - if domain uses BigInt for block numbers, why shouldn't miner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we are using only Longs in EthashUtils. I am not sure if we do that because of performance or sth else, but I don't want to change that in this pr.

@ntallar ntallar requested review from biandratti and removed request for ntallar October 29, 2020 12:58

# Epoch calibration block number
# https://ecips.ethereumclassic.org/ECIPs/ecip-1099
ecip1099-block-number = "2520000"
Copy link
Contributor

Choose a reason for hiding this comment

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

this has already been reached on Mordor, right? Have we synced and try it there already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will sync with the Mordor before the merge

Option(powCaches.get(epoch)) match {
case Some(pcd) => pcd
case None =>
val data = new PowCacheData(cache = EthashUtils.makeCache(epoch), dagSize = EthashUtils.dagSize(epoch))
val data = new PowCacheData(cache = EthashUtils.makeCache(epoch, seed), dagSize = EthashUtils.dagSize(epoch))

val keys = powCaches.keySet().asScala
val keysToRemove = keys.toSeq.sorted.take(keys.size - MaxPowCaches + 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it won't make problems during transition. i.e

  1. lets say we have epoch 358 and 359 here.
  2. we enter fork block ecip-1099 so next epoch is 180 instead of 360
  3. so we put dag cache for this epoch in cache, and we have there dag for epoch 180 and 359 (as we have limit of 2 dag caches max, and we always removes oldest one)
  4. after some blocks we arive at epoch 181. And now we will remove dag cache for epoch 180 instead on 359 .
    It means dag dag cache for epoch 359 will be with us for long time (until we get to new epoch 360), and if rollbacks happen accross new epoch boundaries we will need to recreate dag caches. (we have one stale in there, and we have max size of 2 dag caches )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I fixed that

@mmrozek
Copy link
Contributor Author

mmrozek commented Oct 29, 2020

@jmendiola222, I synced with the Mordor top

@mmrozek mmrozek merged commit 4e2c38c into develop Oct 30, 2020
@mmrozek mmrozek deleted the etcm-109-ecip-1099 branch October 30, 2020 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants