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

Implement bitwise shift opcodes for Constantinople #1112

Merged
merged 3 commits into from
Aug 1, 2018

Conversation

cburgdorf
Copy link
Contributor

@cburgdorf cburgdorf commented Jul 30, 2018

What was wrong?

As stated in #1104, the upcoming Constantinople fork adds three new opcodes for bitwise shifting operations. Namely SHL, SHR and SAR. This PR adds support for the SHL and SHR opcodes.

This does not include support for the SAR opcode yet which is expected to land in another PR. #1134

How was it fixed?

  1. Created a new test_opcodes file and wrote a couple of tests for existing opcodes. This isn't finished but from the perspective of this PR it shouldn't matter and we have an open issue to not forget about testing other existing opcodes Create unit tests for Opcodes #1111

  2. Implemented SHR and SHL logic in eth.vm.logic.arithmetic

  3. Added all test cases from the EIP

  4. Added opcodes that expose the functionality to the constantinople opcodes

Cute Animal Picture

put a cute animal picture link inside the parentheses

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Big 👍 for adding individual opcode function tests.

@cburgdorf cburgdorf force-pushed the christoph/feat/opcodes branch 3 times, most recently from 655a6a2 to d2fc10c Compare July 31, 2018 13:35
@cburgdorf
Copy link
Contributor Author

@pipermerriam failing CI seems like something related to the new network abstraction and unrelated to this PR as far as I can tell. https://circleci.com/gh/ethereum/py-evm/50905?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

@pipermerriam
Copy link
Member

@cburgdorf see cburgdorf#1 for how to do this without the library.

@pipermerriam
Copy link
Member

Regarding test failure: ethereum/pytest-asyncio-network-simulator#6 incoming

@pipermerriam
Copy link
Member

@cburgdorf you should be able to rebase this now and pass CI

@cburgdorf
Copy link
Contributor Author

@pipermerriam yep, passes! There's one more shifting opcode left to implement and I also want to put more work in the tests. I'll try to finish that up tomorrow.

@carver
Copy link
Contributor

carver commented Aug 1, 2018

Can we start running selected Constantinople state tests now?

@pipermerriam
Copy link
Member

Can we start running selected Constantinople state tests now?

@carver yes, but my recommendation would be to do that in a separate PR. We'll need to update the commit that our fixtures are current set at (probably to whatever is in geth master?) and then fix anything that is broken.

@cburgdorf
Copy link
Contributor Author

@pipermerriam @carver Alright, I decided to branch of #1134 for the SAR opcode because I really need to ramp up my unicon bits & bytes skills for that one (Or maybe I just need to pick your brain more @carver 😅)

So, this one is ready for review if anyone is up for it.

@cburgdorf cburgdorf mentioned this pull request Aug 1, 2018
@cburgdorf cburgdorf merged commit 8143710 into ethereum:master Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants