EIP 1283 SSTORE #367

vpulim · 2018-10-08T19:59:32Z

Depends on ethereumjs/ethereumjs-common#27.

Closes #354

I wasn't sure about the best way to test this PR since there aren't any EIP-1283 tests yet in ethereum/tests, so I created my own tests in tests/constantinopleSstoreTest.js until official tests are available. These tests are currently not being run in circleci. Ideas?

coveralls · 2018-10-08T20:13:22Z

Coverage decreased (-1.2%) to 91.677% when pulling 397d510 on eip-1283 into 08b5a93 on master.

holgerd77 · 2018-10-08T20:15:54Z

If I got this correct I think the latest PR merge on the tests repository ethereum/tests#511 should contain various tests (primarily testing other functionality) implicitly testing the new gas prices. This is just three days old, we'll have to do a new release with an updated submodule of our own proxy testing module (ethereumjs-testing).

vpulim · 2018-10-08T20:18:45Z

@holgerd77 Ah ok, great. I didn't realize they were just released. I'll update this PR and remove my tests once ethereumjs-testing is updated.

vpulim · 2018-10-10T21:23:04Z

I just verified that #368 fixes many failing Constantinople state tests using this PR. Still many other failing tests that are dependent on the other open Constantinople PRs.

holgerd77 · 2018-10-11T09:59:35Z

Ok, I just released a new version of the ethereumjs-common library v0.6.0, so this can be updated now.

holgerd77 · 2018-10-11T10:10:39Z

Update: I would update the ethereumjs-common dependency myself and then do a review and eventually merge if you don't intervene in the next hour or so, then we can build/rebase other stuff on top.

Side note: I thought of the labels PR state: WIP and PR state: needs review somewhat excluding each other, so the WIP semantics would be more of "someone is still actively working on it" explicitly leaving out the review as part of the work. With this notion I think it is more useful, then one can switch - once work is ready - from WIP to review label and it's more clear for others what to do. Since you assigned both labels simultaneously I assume that you have done this with the code-work-side of this PR ready and therewith open for a review.

vpulim · 2018-10-11T10:26:10Z

@holgerd77 I'm updating ethereumjs-common to 0.6.0 now, but I'm trying to track down an error that I wasn't seeing before when I ran the Constantinople tests:

# GeneralStateTests
# file: addNonConst test: addNonConst
assert.js:49
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: The field stateRoot must have byte length of 32
    at module.exports.setter [as stateRoot] (/Users/vpulim/dev/ethereumjs-vm/node_modules/ethereumjs-account/node_modules/ethereumjs-util/index.js:644:9)
    at /Users/vpulim/dev/ethereumjs-vm/tests/util.js:384:27

Will report back once I confirm things are okay to proceed.

vpulim · 2018-10-11T10:29:54Z

It looks like #369 is causing these errors. If that is expected behavior due to a dependency on other open PRs, then please proceed with the review/merge.

holgerd77 · 2018-10-11T11:45:38Z

lib/opFns.js

+      current: function (cb) { runState.stateManager.getContractStorage(address, key, cb) }
+    }, cb)
+  } else {
+    runState.stateManager.getContractStorage(address, key, cb)


Pre-constantinople logic seems to be cleanly separated, ok.

holgerd77 · 2018-10-11T11:48:07Z

lib/opFns.js

+    } else if (value.length !== 0 && found.length) {
+      subGas(runState, new BN(runState._common.param('gasPrices', 'sstoreReset')))
+    }
+  }


Same here, clean separation of pre-constantinople SSTORE gas calculations.

holgerd77 · 2018-10-11T11:51:00Z

lib/opFns.js

+      // If current value equals new value (this is a no-op), 200 gas is deducted.
+      subGas(runState, new BN(runState._common.param('gasPrices', 'netSstoreNoopGas')))
+      return
+    }


First case of EIP, with return, ok.

Second look, as far as I see it these both versions 1) first subGas() call, then blank return and 2) return subGas() on other occasions doesn't make any difference I assume (?). Won't make this a review blocker, but unless there are reasons for it I am not seeing right now this might be updated to be more consistent if there is the occasion.

Good catch! This should be return subGas... like in the rest of the code. This is stylistic and can be fixed later... not critical.

holgerd77 · 2018-10-11T12:00:05Z

lib/opFns.js

+        runState.gasRefund = runState.gasRefund.addn(runState._common.param('gasPrices', 'netSstoreResetRefund'))
+      }
+    }
+    return subGas(runState, new BN(runState._common.param('gasPrices', 'netSstoreDirtyGas')))


This is working down the various cases from the EIP and adding to the existing runState.gasRefund variable. I assume this is correct since tests are passing here on a broad basis.

If there are smaller caveats or edge cases we can do subsequent PRs to correct once we have a clearer picture on the test runs and tests have stabilized more.

holgerd77 · 2018-10-11T12:04:12Z

Hi Vinay, have now merged, thanks a lot that you took on this so quickly! 😄

Hmm, the new test failure caused by #369 wasn't intended, but we can then probably address this separately in a new PR, if you want you can file a new issue for that. No need to wait here since #369 is already merged anyhow and we will not do an imminent release on the VM.

holgerd77 · 2018-10-11T12:10:23Z

Short test result summary (ethereumjs-testing v1.2.3, submodule 9777827):

Before:

1..4776
# tests 4776
# pass  3783
# fail  993

After:

1..4776
# tests 4776
# pass  4684
# fail  92

Really cool! 🏆 🏆
And some tropical drink for the win: 🍹 😄

vpulim · 2018-10-11T14:00:18Z

Woo hoo! 🙌

vpulim added PR state: needs review PR state: WIP labels Oct 8, 2018

vpulim force-pushed the eip-1283 branch 2 times, most recently from aebda60 to 9156243 Compare October 8, 2018 20:11

vpulim mentioned this pull request Oct 10, 2018

[Constantinople] EIP 1283 SSTORE net metering #354

Closed

vpulim force-pushed the eip-1283 branch from 9156243 to 8e527e9 Compare October 10, 2018 21:14

holgerd77 mentioned this pull request Oct 11, 2018

Remove explicit direct cache interactions #366

Merged

vpulim mentioned this pull request Oct 11, 2018

[Constantinople] Add Constantinople SSTORE gas/refund prices (EIP1283) ethereumjs/ethereumjs-common#27

Merged

vpulim removed the PR state: WIP label Oct 11, 2018

EIP 1283 SSTORE

397d510

vpulim force-pushed the eip-1283 branch from 8e527e9 to 397d510 Compare October 11, 2018 10:31

holgerd77 approved these changes Oct 11, 2018

View reviewed changes

holgerd77 merged commit 7e15b49 into master Oct 11, 2018

holgerd77 deleted the eip-1283 branch October 11, 2018 12:01

This was referenced Oct 11, 2018

EIP 1014 CREATE2 #329

Merged

Constantinople Progress Tracker ethereum/pm#53

Closed

mattdean-digicatapult mentioned this pull request Nov 14, 2018

StateManager interface simplification #388

Merged

This was referenced Jan 23, 2019

Support pre-/post reentrency Constantinople #423

Closed

Updated pre-/post reentrency Constantinople support ethereumjs/ethereumjs-common#41

Closed

danjm mentioned this pull request Feb 1, 2019

Add petersburg aka constantinopleFix support #433

Merged

holgerd77 mentioned this pull request Aug 21, 2019

Istanbul Hardfork Meta Issue #501

Closed

18 tasks

evertonfraga added the package: vm label Feb 27, 2020

Woodpile37 mentioned this pull request Aug 25, 2023

[Snyk] Upgrade eslint-plugin-sonarjs from 0.14.0 to 0.19.0 Woodpile37/ethereumjs-monorepo#30

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP 1283 SSTORE #367

EIP 1283 SSTORE #367

vpulim commented Oct 8, 2018 •

edited

Loading

coveralls commented Oct 8, 2018 •

edited

Loading

holgerd77 commented Oct 8, 2018

vpulim commented Oct 8, 2018 •

edited

Loading

vpulim commented Oct 10, 2018

holgerd77 commented Oct 11, 2018

holgerd77 commented Oct 11, 2018

vpulim commented Oct 11, 2018

vpulim commented Oct 11, 2018

holgerd77 Oct 11, 2018

holgerd77 Oct 11, 2018

holgerd77 Oct 11, 2018

holgerd77 Oct 11, 2018

vpulim Oct 11, 2018

holgerd77 Oct 11, 2018

holgerd77 commented Oct 11, 2018

holgerd77 commented Oct 11, 2018

vpulim commented Oct 11, 2018

EIP 1283 SSTORE #367

EIP 1283 SSTORE #367

Conversation

vpulim commented Oct 8, 2018 • edited Loading

coveralls commented Oct 8, 2018 • edited Loading

holgerd77 commented Oct 8, 2018

vpulim commented Oct 8, 2018 • edited Loading

vpulim commented Oct 10, 2018

holgerd77 commented Oct 11, 2018

holgerd77 commented Oct 11, 2018

vpulim commented Oct 11, 2018

vpulim commented Oct 11, 2018

holgerd77 Oct 11, 2018

Choose a reason for hiding this comment

holgerd77 Oct 11, 2018

Choose a reason for hiding this comment

holgerd77 Oct 11, 2018

Choose a reason for hiding this comment

holgerd77 Oct 11, 2018

Choose a reason for hiding this comment

vpulim Oct 11, 2018

Choose a reason for hiding this comment

holgerd77 Oct 11, 2018

Choose a reason for hiding this comment

holgerd77 commented Oct 11, 2018

holgerd77 commented Oct 11, 2018

vpulim commented Oct 11, 2018

vpulim commented Oct 8, 2018 •

edited

Loading

coveralls commented Oct 8, 2018 •

edited

Loading

vpulim commented Oct 8, 2018 •

edited

Loading