Skip to content

[ETCM-109] ECIP-1099 implementation#764

Merged
mmrozek merged 6 commits intodevelopfrom
etcm-109-ecip-1099
Oct 30, 2020
Merged

[ETCM-109] ECIP-1099 implementation#764
mmrozek merged 6 commits intodevelopfrom
etcm-109-ecip-1099

Conversation

@mmrozek
Copy link
Copy Markdown
Contributor

@mmrozek mmrozek commented Oct 29, 2020

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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