-
Notifications
You must be signed in to change notification settings - Fork 75
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
[EC-231] ECIP 1017 Geth compatibility fixes and tests #307
Changes from 6 commits
696606f
3158510
a537a20
04ca1e0
334dd34
2b101e2
905a022
e348412
ee76e15
4f1d4bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
package io.iohk.ethereum.txExecTest | ||
|
||
import akka.util.ByteString | ||
import io.iohk.ethereum.domain.{BlockchainImpl, Receipt, UInt256} | ||
import io.iohk.ethereum.ledger.LedgerImpl | ||
import io.iohk.ethereum.txExecTest.util.FixtureProvider | ||
import io.iohk.ethereum.utils.{BlockchainConfig, MonetaryPolicyConfig} | ||
import io.iohk.ethereum.validators._ | ||
import io.iohk.ethereum.vm.VM | ||
import org.scalatest.{FlatSpec, Matchers} | ||
|
||
class ECIP1017Test extends FlatSpec with Matchers { | ||
|
||
val EraDuration = 3 | ||
|
||
val blockchainConfig = new BlockchainConfig { | ||
override val monetaryPolicyConfig: MonetaryPolicyConfig = MonetaryPolicyConfig(EraDuration, 0.2, 5000000000000000000L) | ||
|
||
// unused | ||
override val chainId: Byte = 0x3d | ||
override val frontierBlockNumber: BigInt = 0 | ||
override val homesteadBlockNumber: BigInt = 1150000 | ||
override val eip150BlockNumber: BigInt = 2500000 | ||
override val eip160BlockNumber: BigInt = 3000000 | ||
override val eip155BlockNumber: BigInt = 3000000 | ||
override val eip106BlockNumber: BigInt = Long.MaxValue | ||
override val customGenesisFileOpt: Option[String] = None | ||
override val daoForkBlockNumber: BigInt = 1920000 | ||
override val daoForkBlockHash: ByteString = ByteString("unused") | ||
override val difficultyBombPauseBlockNumber: BigInt = Long.MaxValue | ||
override val difficultyBombContinueBlockNumber: BigInt = Long.MaxValue | ||
override val accountStartNonce: UInt256 = UInt256.Zero | ||
} | ||
|
||
val noErrors = a[Right[_, Seq[Receipt]]] | ||
|
||
val validators = new Validators { | ||
val blockValidator: BlockValidator = BlockValidator | ||
val blockHeaderValidator: BlockHeaderValidator = new BlockHeaderValidatorImpl(blockchainConfig) | ||
val ommersValidator: OmmersValidator = new OmmersValidatorImpl(blockchainConfig) | ||
val signedTransactionValidator: SignedTransactionValidator = new SignedTransactionValidatorImpl(blockchainConfig) | ||
} | ||
|
||
"Ledger" should "execute blocks with respect to block reward changed by ECIP 1017" in { | ||
val fixtures: FixtureProvider.Fixture = FixtureProvider.loadFixtures("/txExecTest/ecip1017Test") | ||
|
||
val startBlock = 1 | ||
val endBlock = 602 | ||
|
||
(startBlock to endBlock) foreach { blockToExecute => | ||
val storages = FixtureProvider.prepareStorages(blockToExecute - 1, fixtures) | ||
val blockchain = BlockchainImpl(storages) | ||
val ledger = new LedgerImpl(VM, blockchain, blockchainConfig) | ||
|
||
ledger.executeBlock(fixtures.blockByNumber(blockToExecute), validators) shouldBe noErrors | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,10 @@ import io.iohk.ethereum.utils.MonetaryPolicyConfig | |
|
||
/** | ||
* Calculates rewards for mining blocks and ommers. | ||
* Avoids floating point arithmetic. Because of that the formulas may look a bit unintuitive, but the important | ||
* thing here is that we want to defer any division to be a single and final operation | ||
* To be compatible with Geth division was not defered to be a single and final operation, which produces expected | ||
* rounding errors. | ||
* A Geth issue (ethereumproject/go-ethereum#352) and a comment on the ECIP (https://github.com/ethereumproject/ECIPs/issues/15#issuecomment-330660976) | ||
* were created to check whether this should be changed | ||
*/ | ||
class BlockRewardCalculator(config: MonetaryPolicyConfig) { | ||
/** Era duration in blocks */ | ||
|
@@ -41,12 +43,9 @@ class BlockRewardCalculator(config: MonetaryPolicyConfig) { | |
|
||
def calcBlockMinerReward(blockNumber: BigInt, ommersCount: Int): BigInt = { | ||
val era = eraNumber(blockNumber) | ||
val eraMultiplier = rewardReductionRateNumer.pow(era) | ||
val eraDivisor = rewardReductionRateDenom.pow(era) | ||
|
||
val baseReward = (firstEraBlockReward * eraMultiplier) / eraDivisor | ||
val ommersReward = (firstEraBlockReward * ommersCount * ommerInclusionRewardNumer * eraMultiplier) / | ||
(ommerInclusionRewardDenom * eraDivisor) | ||
val baseReward = calcMinerBaseReward(era) | ||
val ommersReward = calcMinerRewardPerOmmer(era) * ommersCount | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Take a note of ethereumproject/go-ethereum#352 and ethereumproject/ECIPs#15 (comment). Maybe we should at least add comments with those links until the issue is resolved. Ideally we should hold on merging this until we get some feedback (hopefully soon) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we go with this implementation, comments need to be updated to reflect that these are separate rewards that have already been rounded down. |
||
baseReward + ommersReward | ||
} | ||
|
||
|
@@ -56,13 +55,31 @@ class BlockRewardCalculator(config: MonetaryPolicyConfig) { | |
if (era == 0) { | ||
val numer = firstEraOmmerMiningRewardMaxNumer - (blockNumber - ommerNumber - 1) | ||
(firstEraBlockReward * numer) / firstEraOmmerMiningRewardDenom | ||
} else { | ||
val eraMultiplier = rewardReductionRateNumer.pow(era) | ||
val eraDivisor = rewardReductionRateDenom.pow(era) | ||
(firstEraBlockReward * ommerMiningRewardNumer * eraMultiplier) / (ommerMiningRewardDenom * eraDivisor) | ||
} | ||
} else | ||
calcMinerBaseReward(era) * ommerMiningRewardNumer / ommerMiningRewardDenom | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're multiplying the result of division, thus introducing rounding errors There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this could cause rounding error, however as this seems to be the more closer approach to how it is calculated in Geth and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, if geth does it then it's fine. I thought about it and it's not devoid of logic to have it rounded down prior to another division. |
||
} | ||
|
||
/** | ||
* Calculates the miner base reward (without considering the ommers included) | ||
* | ||
* @param era to which the mined block belongs | ||
* @return miner base reward | ||
*/ | ||
private def calcMinerBaseReward(era: Int): BigInt = { | ||
val eraMultiplier = rewardReductionRateNumer.pow(era) | ||
val eraDivisor = rewardReductionRateDenom.pow(era) | ||
firstEraBlockReward * eraMultiplier / eraDivisor | ||
} | ||
|
||
/** | ||
* Calculates reward given to the miner for each ommer included in the block | ||
* | ||
* @param era to which the mined block belongs | ||
* @return reward given to the miner for each ommer included | ||
*/ | ||
private def calcMinerRewardPerOmmer(era: Int): BigInt = | ||
calcMinerBaseReward(era) * ommerInclusionRewardNumer / ommerInclusionRewardDenom | ||
|
||
/** era number counting from 0 */ | ||
private def eraNumber(blockNumber: BigInt): Int = | ||
((blockNumber - 1) / eraDuration).toInt | ||
|
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.
Could we maybe add a comment about era 193 going zero, and how we want to go past that point.