[EC-231] ECIP 1017 Geth compatibility fixes and tests #307
Conversation
| val ommersReward = (firstEraBlockReward * ommersCount * ommerInclusionRewardNumer * eraMultiplier) / | ||
| (ommerInclusionRewardDenom * eraDivisor) | ||
| val baseReward = calcMinerBaseReward(era) | ||
| val ommersReward = calcMinerRewardPerOmmer(era) * ommersCount |
There was a problem hiding this comment.
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.
If we go with this implementation, comments need to be updated to reflect that these are separate rewards that have already been rounded down.
| (firstEraBlockReward * ommerMiningRewardNumer * eraMultiplier) / (ommerMiningRewardDenom * eraDivisor) | ||
| } | ||
| } else | ||
| calcMinerBaseReward(era) * ommerMiningRewardNumer / ommerMiningRewardDenom |
There was a problem hiding this comment.
You're multiplying the result of division, thus introducing rounding errors
There was a problem hiding this comment.
Yes, this could cause rounding error, however as this seems to be the more closer approach to how it is calculated in Geth and ommerMiningRewardNumer=1, I decided to leave it this way.
Do you think it would be better to change this?
There was a problem hiding this comment.
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.
| val fixtures: FixtureProvider.Fixture = FixtureProvider.loadFixtures("/txExecTest/ecip1017Test") | ||
|
|
||
| val startBlock = 1 | ||
| val endBlock = 100 |
There was a problem hiding this comment.
Could we have more blocks in this test? The further the era, the more chance of catching a rounding error. Ideally we should reach era #170 - IIRC, that's when rewards drop to zero.
There was a problem hiding this comment.
Era #193 actually:
x = 1 - floor(ln(a) / ln(1 - m))
where:
x - the era number where rewards drop to zero
a - base reward (5 Ether)
m - reduction rate (0.2)
There was a problem hiding this comment.
Can we make it an even 200 to be sure? 😃
There was a problem hiding this comment.
Changed endBlock to 602, which is the first block that belongs to era 200.
No other compatibility error was found 😄
…s and added comments
…to feature/testECIP1017
| val fixtures: FixtureProvider.Fixture = FixtureProvider.loadFixtures("/txExecTest/ecip1017Test") | ||
|
|
||
| val startBlock = 1 | ||
| val endBlock = 602 |
There was a problem hiding this comment.
Could we maybe add a comment about era 193 going zero, and how we want to go past that point.
|
Could we maybe add some test cases to |
… integration test
ntallar
dismissed stale reviews from KonradStaniec and rtkaczyk
via
4f1d4bd
Description
Includes integration tests that executes blocks generated by Geth, for which the
eraDurationwas reduced to 3. The purpose of these was to test the compatibility of Mantis with Geth, for which several changes were done to theBlockRewardCalculator, based on the formulas used on Geth.During testing it was found that Geth and Parity aren't compatible, which is why this test doesn't check compatibility with Parity.