Implement bitwise shifting #251

axic · 2018-01-26T15:02:58Z

Based on https://github.com/ethereum/EIPs/blob/master/EIPS/eip-145.md

axic · 2018-01-26T15:18:29Z

Fails because of the stBadOpcodes test.

axic · 2018-01-26T16:46:45Z

@holgerd77 @cdetrio @jwasinger this would be enabled for constantinople only. How to do it?

holgerd77 · 2018-01-26T19:55:41Z

@axic Do you want to have this already implemented for testing purposes or anything? Or can't we just leave the PR open until Constantinople is more in sight?

axic · 2018-01-26T20:28:58Z

No need to merge yet until there are some state tests for it and it is fully implemented. Not sure even at that point how can we merge it since there's only one version supported: the last.

But if you want you can implement the SHR part and review the rest based on the EIP.

holgerd77 · 2018-07-11T08:13:11Z

Just rebased this.

axic · 2018-07-20T10:29:46Z

Using #304 features here now.

axic · 2018-07-20T10:30:10Z

@holgerd77 do you want to check if the API is used correctly?

holgerd77 · 2018-07-20T10:57:48Z

This isn't prepared to work with block numbers yet (also since in a simulated environment (Truffle,...) block numbers are not correctly corresponding to the desired HF behaviour), this is something for later down the road once this becomes relevant for the client.

For now this compares against the hardfork set in the common class, so do something like:

if (runState._common.gteHardfork('constantinople') {
   // Do the constantinople stuff here
}

I generally had the idea to refactor the opFns.js file and directory structure a bit for this with the following structure in mind, replacing the current lib/opFns.js file:

lib/opFns/byzantium.js Includes (for now) all the current opcodes for Byzantium, might be split further in the future
lib/opFns/constantinople.js New Constantinople opcodes
lib/util.js all the generic functions from lib/opFns.js

With this we would have a cleaner structure and separation and could add the opcodes for the HF selected in runCode.js with something like:

if (runState._common.gteHardfork('constantinople') {
  opFns = Object.assign({}, require('lib/opFns/constantinople.js'))
}

(First shot, not thoroughly thought through)
and the do the checks there centrally.

What do you think?

holgerd77 · 2018-07-20T13:12:29Z

Regarding the test setup we can also now running at least the state tests for both hardforks in parallel (especially with the now fast-running Circle tests) and keep the Byzantium tests activated to be mandatory for passing CI until all Constantinople tests pass.

So we don't have to change the hardfork setting in the tester file directly, but we can introduce another package.json script + Circle task, maybe something generic to be future-ready like:

{
  "testState": "npm run build:dist && node ./tests/tester -s --dist --fork='byzantium'",
  "testStateDev": "npm run build:dist && node ./tests/tester -s --dist --fork='constantinople"",
}

...plus a new Circle job test_state_dev. And in the protected branch setting we let the dev tests run but don't mark them as mandatory. Like this we always have an overview on the increase of Constantinople tests passing while making sure to not break the current Byzantium build.

axic · 2018-07-23T18:53:01Z

I generally had the idea to refactor the opFns.js file and directory structure a bit for this with the following structure in mind, replacing the current lib/opFns.js file:

I think that idea makes sense from an optimisation perspective, but right now I would think that adding single conditions achieve the goal, in terms of both readability and speed of implementing the new features.

axic · 2018-07-23T18:53:53Z

This isn't prepared to work with block numbers yet (also since in a simulated environment (Truffle,...) block numbers are not correctly corresponding to the desired HF behaviour), this is something for later down the road once this becomes relevant for the client.

In that case I'd think a helper on runState could do the trick: runState.hardforkEnabled('...'). That helps the block number logic to be hidden in a higher level.

holgerd77 · 2018-07-23T18:59:49Z

@axic I think there should be currently no needs for something to be hidden if we use the API methods I describe above, like

if (runState._common.gteHardfork('constantinople') {
   // Do the constantinople stuff here
}

Or am I missing your point?

axic · 2018-07-23T19:00:47Z

The block number question could be solved (and decided) by the caller in that case.

holgerd77 · 2018-07-23T19:04:27Z

Atm I don't really see a "block number question"? Or do you already want to start integrating dynamic fork switch support depending on block number? At the moment I would envision this for just statically instantiate the VM with Byzantium or Constantinople, and then everything is set. Or am I targeting too low?

axic · 2018-07-23T19:29:49Z

Didn't know gteHardfork existed (it is not documented). Looks good for this usecase.

coveralls · 2018-07-23T19:37:59Z

Coverage decreased (-0.8%) to 84.814% when pulling d843cd2 on bitwise into 8d5f330 on master.

axic · 2018-07-23T19:39:20Z

Since SAR was not implemented and wasn't returning a stack item, the stack check code in runCode kicked in, but it was never tested properly and had missing instantiation of VMError:

 file: sar01 test: sar01
/home/circleci/project/ethereumjs-vm/dist/exceptions.js:15
  this.error = error;
             ^

TypeError: Cannot set property 'error' of undefined
    at VmError (/home/circleci/project/ethereumjs-vm/dist/exceptions.js:15:14)
    at runOp (/home/circleci/project/ethereumjs-vm/dist/runCode.js:221:21)

holgerd77 · 2018-07-23T19:53:40Z

gteHardfork docs: https://github.com/ethereumjs/ethereumjs-common/blob/master/docs/index.md#gtehardfork

axic · 2018-07-23T19:57:43Z

This passes the constantinople tests now, woohoo!

axic · 2018-07-23T20:11:26Z

Reason for coverage decrease: coveralls is only running on test_state_byzantium.

holgerd77 · 2018-07-24T10:32:37Z

Congrats! 👍

Just double checked that all the relevant tests are passing, these should be sar_*, shl_* and shr_* I suppose.

We should really change the test summary behavior so that skipped tests are not displayed as passed ones - which currently is the case. Will see if I can make a PR here, shouldn't be that much of a change.

axic · 2018-07-24T10:49:42Z

We should really change the test summary behavior so that skipped tests are not displayed as passed ones - which currently is the case. Will see if I can make a PR here, shouldn't be that much of a change.

Should that be part of this PR?

holgerd77 · 2018-07-24T10:58:16Z

Ah, just checked this manually, changing t.comment(`No ${options.forkConfig} post state defined, skip test`) in GeneralStateTestRunner.js to t.skip(...) doesn't make things better, since this is then counted as passed.

I was actually wrong on this, skipped tests are atm not counted or reported at all. This is an open (for a long time) issue on tape: tape-testing/tape#197

So we would have to manually add a counter or something, seems not worth it for the moment, this should better be added on tape itself (hopefully sometime).

axic · 2018-07-24T11:16:31Z

So can this be merged?

holgerd77

Looks good.

holgerd77 · 2018-07-24T11:07:15Z

lib/opFns.js

+    if (a.gten(256)) {
+      return new BN(0)
+    }
+    return b.shln(a.toNumber()).iand(utils.MAX_INTEGER)


https://github.com/ethereum/EIPs/blob/master/EIPS/eip-145.md#0x1b-shl-shift-left

I am not super-fluent on bit operations, but I think I have found all conditions from the EIP in the implementation.

(always a bit shaky on this signed/unsigned stuff)

holgerd77 · 2018-07-24T11:08:07Z

lib/opFns.js

+    if (a.gten(256)) {
+      return new BN(0)
+    }
+    return b.shrn(a.toNumber())


https://github.com/ethereum/EIPs/blob/master/EIPS/eip-145.md#0x1c-shr-logical-shift-right

Same for SHR.

holgerd77 · 2018-07-24T11:20:40Z

lib/opFns.js

+    } else {
+      return c
+    }
+  },


https://github.com/ethereum/EIPs/blob/master/EIPS/eip-145.md#0x1d-sar-arithmetic-shift-right

I also think I can roughly follow what is going on here and think all EIP conditions are covered and can also place all - this to me appearing a bit esoteric - mask stuff to the places in the EIP. Would have to do an extra hour of bit operation reading though for a better understanding.

Since tests are passing I will approve.

Here the point is to fill the top with set bits, that is achieved by ORing the mask. The mask is then created by taking an all bits set input (MAX_INTEGER) and making sure the bottom part where actual data remainded is 0.

holgerd77 · 2018-07-24T11:23:06Z

Ah, just saw your question, did the review before. If you rebase, I can do a quick re-approval and we can merge if you want.

…entation

axic · 2018-07-24T12:01:25Z

@holgerd77 Rebased.

holgerd77

Renewed approval.

axic added the in progress label Jan 26, 2018

holgerd77 self-assigned this Jan 30, 2018

axic force-pushed the bitwise branch from cffc2d0 to b0398e5 Compare January 30, 2018 12:55

holgerd77 force-pushed the bitwise branch from b518bb4 to d75c1f0 Compare July 11, 2018 08:12

axic force-pushed the bitwise branch from d75c1f0 to 53b6e14 Compare July 20, 2018 10:29

axic force-pushed the bitwise branch from 53b6e14 to 7cbff47 Compare July 20, 2018 10:36

axic force-pushed the bitwise branch from 7cbff47 to 0540c16 Compare July 23, 2018 19:29

axic force-pushed the bitwise branch 2 times, most recently from 24751c6 to 76da77d Compare July 23, 2018 19:31

axic force-pushed the bitwise branch from 4623380 to d1a0359 Compare July 23, 2018 19:41

axic force-pushed the bitwise branch from 4f4b88e to 93835d2 Compare July 23, 2018 19:54

axic force-pushed the bitwise branch from 93835d2 to fb8faac Compare July 23, 2018 20:00

axic changed the title ~~[WIP] Implement bitwise shifting~~ Implement bitwise shifting Jul 23, 2018

axic requested a review from holgerd77 July 23, 2018 20:02

holgerd77 previously approved these changes Jul 24, 2018

View reviewed changes

axic added 6 commits July 24, 2018 13:01

Add SHL/SHR/SAR opcodes

85aad21

Implement SHL and SHR

ed6b9dc

Check if constantinople hard fork is active in the SHL/SHR/SAR implem…

6218bc6

…entation

Implement SAR

4c64288

Always properly throw VMError

b90ba6e

Optimise SAR to not use from/toTwos conversion

d843cd2

axic dismissed holgerd77’s stale review via d843cd2 July 24, 2018 12:01

axic force-pushed the bitwise branch from fb8faac to d843cd2 Compare July 24, 2018 12:01

holgerd77 approved these changes Jul 24, 2018

View reviewed changes

holgerd77 merged commit 9413298 into master Jul 24, 2018

holgerd77 deleted the bitwise branch July 24, 2018 12:13

jwasinger mentioned this pull request Jul 30, 2018

Constantinople Implementation Progress #322

Closed

5 tasks

Souptacular mentioned this pull request Aug 4, 2018

Constantinople Progress Tracker ethereum/pm#53

Closed

evertonfraga added the package: vm label Feb 27, 2020

Woodpile37 mentioned this pull request Aug 25, 2023

[Snyk] Upgrade eslint-config-prettier from 8.5.0 to 8.10.0 Woodpile37/ethereumjs-monorepo#28

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement bitwise shifting #251

Implement bitwise shifting #251

axic commented Jan 26, 2018

axic commented Jan 26, 2018

axic commented Jan 26, 2018

holgerd77 commented Jan 26, 2018

axic commented Jan 26, 2018 •

edited

Loading

holgerd77 commented Jul 11, 2018

axic commented Jul 20, 2018

axic commented Jul 20, 2018

holgerd77 commented Jul 20, 2018

holgerd77 commented Jul 20, 2018

axic commented Jul 23, 2018

axic commented Jul 23, 2018

holgerd77 commented Jul 23, 2018

axic commented Jul 23, 2018

holgerd77 commented Jul 23, 2018

axic commented Jul 23, 2018

coveralls commented Jul 23, 2018 •

edited

Loading

axic commented Jul 23, 2018

holgerd77 commented Jul 23, 2018

axic commented Jul 23, 2018

axic commented Jul 23, 2018

holgerd77 commented Jul 24, 2018

axic commented Jul 24, 2018

holgerd77 commented Jul 24, 2018

axic commented Jul 24, 2018

holgerd77 left a comment

holgerd77 Jul 24, 2018

holgerd77 Jul 24, 2018

holgerd77 Jul 24, 2018

holgerd77 Jul 24, 2018

axic Jul 24, 2018 •

edited

Loading

holgerd77 commented Jul 24, 2018

axic commented Jul 24, 2018

holgerd77 left a comment

Implement bitwise shifting #251

Implement bitwise shifting #251

Conversation

axic commented Jan 26, 2018

axic commented Jan 26, 2018

axic commented Jan 26, 2018

holgerd77 commented Jan 26, 2018

axic commented Jan 26, 2018 • edited Loading

holgerd77 commented Jul 11, 2018

axic commented Jul 20, 2018

axic commented Jul 20, 2018

holgerd77 commented Jul 20, 2018

holgerd77 commented Jul 20, 2018

axic commented Jul 23, 2018

axic commented Jul 23, 2018

holgerd77 commented Jul 23, 2018

axic commented Jul 23, 2018

holgerd77 commented Jul 23, 2018

axic commented Jul 23, 2018

coveralls commented Jul 23, 2018 • edited Loading

axic commented Jul 23, 2018

holgerd77 commented Jul 23, 2018

axic commented Jul 23, 2018

axic commented Jul 23, 2018

holgerd77 commented Jul 24, 2018

axic commented Jul 24, 2018

holgerd77 commented Jul 24, 2018

axic commented Jul 24, 2018

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 Jul 24, 2018

Choose a reason for hiding this comment

holgerd77 Jul 24, 2018

Choose a reason for hiding this comment

holgerd77 Jul 24, 2018

Choose a reason for hiding this comment

holgerd77 Jul 24, 2018

Choose a reason for hiding this comment

axic Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

holgerd77 commented Jul 24, 2018

axic commented Jul 24, 2018

holgerd77 left a comment

Choose a reason for hiding this comment

axic commented Jan 26, 2018 •

edited

Loading

coveralls commented Jul 23, 2018 •

edited

Loading

axic Jul 24, 2018 •

edited

Loading