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

Update ethereum-tests submodule to 8.0.0 / Remove EIP-2315 from berlin #1142

Merged
merged 6 commits into from
Mar 8, 2021

Conversation

holgerd77
Copy link
Member

This is a simple PR which updates the ethereum-tests submodule to the Berlin release 8.0.0 with a removed EIP-2315 HF. EIP-2315 is also removed here in Common in the berlin HF definition file.

I originally also wanted to add the block numbers for berlin along this PR. But I don't trust these numbers yet since the Ropsten HF would be so close and there is no corresponding go-ethereum release out yet, see also ethereum/pm#248 (comment).

@holgerd77 holgerd77 added PR state: needs review type: tests type: enhancement package: vm type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. labels Mar 8, 2021
@holgerd77
Copy link
Member Author

Note that this PR is also running with the type: test all hardforks label.

@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #1142 (ffaf9cb) into master (94651f7) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 81.82% <ø> (+0.21%) ⬆️
blockchain 84.19% <ø> (+0.06%) ⬆️
client 87.04% <ø> (+0.19%) ⬆️
common 86.47% <ø> (-1.41%) ⬇️
devp2p 84.28% <ø> (ø)
ethash 82.08% <ø> (ø)
tx 84.86% <ø> (+0.18%) ⬆️
vm 81.67% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77 holgerd77 force-pushed the update-tests-and-berlin-HF branch from 66a4608 to 03d00fd Compare March 8, 2021 08:42
@holgerd77
Copy link
Member Author

Some tests failing, feel very free to directly push here. Will stop for now.

@jochem-brouwer
Copy link
Member

Have given this a quick glance, this is probably the problem. But I don't have time today to actually check it.

@jochem-brouwer
Copy link
Member

This should fix the problem. Have also moved EIP2315 tests from berlin directory into EIPs directory.

@holgerd77 as for why your tests passed locally: you probably forgot to rebuild Common, so if you would create a Berlin common it automatically activates EIP2315.

@holgerd77
Copy link
Member Author

@jochem-brouwer lol, good point 😄 (yes, you're right)

@holgerd77
Copy link
Member Author

@jochem-brouwer we can now also include the Berlin HF blocks here along since this is now settled. Do you might want to directly do, don't want to push here in between?

@jochem-brouwer
Copy link
Member

Ah right, good point, will add them here and push.

@holgerd77
Copy link
Member Author

@jochem-brouwer cool! 👍

Have also cross-checked the EIP-2315 implementation, I think we've all covered now.

@jochem-brouwer
Copy link
Member

Right, I don't have the fork hashes, and sadly we also don't have an utility module to generate these fork hashes (do we want this?). I have asked on R&D if someone has the hashes for me.

@holgerd77
Copy link
Member Author

@jochem-brouwer ah, you are not reading my messages 😋 - also wrote about this in the Yolov3 PR - you can use the Common._calcForkHash() method for that.

@holgerd77
Copy link
Member Author

(so put in the block numbers and then run with a Common set to berlin from console)

@jochem-brouwer
Copy link
Member

Ah I missed this message, just figured this out as well that we actually have this feature 😅

@jochem-brouwer
Copy link
Member

jochem-brouwer commented Mar 8, 2021

Found a possible problem: we do not pad the fork hashes, which is why for instance this kovan hash is of 7 characters (9 characters with 0x prefixed). This is problematic if we do Buffer.from(hash.slice(2)), you'd get the Buffer 10ffe5, where we expect Buffer 010ffe56.

I don't know if this is an issue, but it is not consistent with other packages, we usually left pad these strings or Buffers.

Anyways, have added Berlin blocks to Common, see the EF blog for block numbers. Also got the fork hashes from Geth's fork hash tests. And I also learned we can actually calculate these hashes 😅

Copy link
Member Author

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Ok, double-checked the numbers, LGTM

@holgerd77 holgerd77 merged commit 8201891 into master Mar 8, 2021
@holgerd77 holgerd77 deleted the update-tests-and-berlin-HF branch March 8, 2021 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: vm PR state: needs review type: enhancement type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. type: tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants