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

consensus, core, tests: implement Metropolis EIP 649 #15028

Merged
merged 1 commit into from Aug 25, 2017

Conversation

Projects
None yet
4 participants
@karalabe
Member

karalabe commented Aug 24, 2017

Implements ethereum/EIPs#669.

The PR also pulls in the latest test suite, which among other throws out explicit log validation to implicit one via RLP(logs).


Note, some tests are failing because the consensus test suite was only partially updated to the new log format, so the non-updated ones cannot be parsed. I'll keep checking the test suite to see if there are updates to fix them.

@karalabe karalabe added the metropolis label Aug 24, 2017

@karalabe karalabe added this to the 1.7.0 milestone Aug 24, 2017

@karalabe karalabe requested a review from holiman Aug 24, 2017

PostStateRoot common.Hash `json:"postStateRoot"`
Env stEnv `json:"env"`
Exec vmExec `json:"exec"`
Logs common.UnprefixedHash `json:"logs"`

This comment has been minimized.

@fjl

fjl Aug 24, 2017

Contributor

Please remove the logs check so we can merge.

@fjl

fjl Aug 24, 2017

Contributor

Please remove the logs check so we can merge.

This comment has been minimized.

@karalabe

karalabe Aug 24, 2017

Member

I'll wait to see how many fail (just updates the test suite), and will disable only the failing tests (so we're not overly zealous and accidentally miss consensus issues).

@karalabe

karalabe Aug 24, 2017

Member

I'll wait to see how many fail (just updates the test suite), and will disable only the failing tests (so we're not overly zealous and accidentally miss consensus issues).

@holiman

This comment has been minimized.

Show comment
Hide comment
@holiman

holiman Aug 24, 2017

Contributor

This was already here, not new for this PR: https://github.com/ethereum/go-ethereum/pull/15028/files#diff-e226000964244889df127e5b44dbf192R546 :

 		r.Div(blockReward, big32)
 		reward.Add(reward, r)

Looks un-intuitive (reusing r which has a totally different meaning on the lines before) and unnecessary (always divide blockReward by 32).
I'd prefer:


	blockReward := frontierBlockReward
        newphewReward := frontierNewphewReward
 	if config.IsMetropolis(header.Number) {
 		blockReward = metropolisBlockReward
                nephewReward:= metropolisNephewReward
 	}
       ...
 		reward.Add(reward, nephewReward)
Contributor

holiman commented Aug 24, 2017

This was already here, not new for this PR: https://github.com/ethereum/go-ethereum/pull/15028/files#diff-e226000964244889df127e5b44dbf192R546 :

 		r.Div(blockReward, big32)
 		reward.Add(reward, r)

Looks un-intuitive (reusing r which has a totally different meaning on the lines before) and unnecessary (always divide blockReward by 32).
I'd prefer:


	blockReward := frontierBlockReward
        newphewReward := frontierNewphewReward
 	if config.IsMetropolis(header.Number) {
 		blockReward = metropolisBlockReward
                nephewReward:= metropolisNephewReward
 	}
       ...
 		reward.Add(reward, nephewReward)
@devsnd

This comment has been minimized.

Show comment
Hide comment
@holiman

This comment has been minimized.

Show comment
Hide comment
@holiman

holiman Aug 24, 2017

Contributor

@devsnd OMG wow good catch!

It's using 299999 instead of 2999999

Contributor

holiman commented Aug 24, 2017

@devsnd OMG wow good catch!

It's using 299999 instead of 2999999

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe Aug 24, 2017

Member

@fjl I've disabled the 3 failing test files, we can readd them later.
@holiman I've fixed the 2_999_999 issue, PLTA.
@devsnd 2,999,999 is correct because the code there is working on the parent block number, not the actual block number. Though do ping me if you still think it's wrong.

Member

karalabe commented Aug 24, 2017

@fjl I've disabled the 3 failing test files, we can readd them later.
@holiman I've fixed the 2_999_999 issue, PLTA.
@devsnd 2,999,999 is correct because the code there is working on the parent block number, not the actual block number. Though do ping me if you still think it's wrong.

@holiman

This comment has been minimized.

Show comment
Hide comment
@holiman

holiman Aug 24, 2017

Contributor

Just to be clear, my initial comment above was to point out that it was supposed to be one less than 3M, but then I understood that @devsnd had correctly spotted an error - so again, good catch!

Contributor

holiman commented Aug 24, 2017

Just to be clear, my initial comment above was to point out that it was supposed to be one less than 3M, but then I understood that @devsnd had correctly spotted an error - so again, good catch!

@holiman

This comment has been minimized.

Show comment
Hide comment
@holiman

holiman Aug 24, 2017

Contributor

PTAL:ed, looks good to me

Contributor

holiman commented Aug 24, 2017

PTAL:ed, looks good to me

@karalabe karalabe merged commit 27a5622 into ethereum:master Aug 25, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
commit-message-check/gitcop All commit messages are valid
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment