Support pre-/post reentrency Constantinople

[holgerd77]

Context is described in the issue on the ethereumjs-common library this issue depends on.

For full support we have to change SSTORE functionality added by @vpulim in the PR here #367 to activate on constantinopleTmp and beyond and deactivate again on constantinople and beyond.

Since the PR above also came along with some heavy (local) refactoring changes, we should apply this not by just reverting but by taking these into account and making this actually more elegant than it was before. 😄

Depends on ethereumjs/ethereumjs-common#41.

[danjm]

@holgerd77 I feel like I am missing something. It seems that the changes done in #367, along with your ethereumjs-common PR (ethereumjs/ethereumjs-common#44), have already properly isolated EIP-1283.

#367 placed the constantinople EIP-1283 code inside if block where the the condition for running the code is if (runState._common.gteHardfork('constantinople')) {. This will be false before constantinople and after (i.e. on petersburg/constantinopleFix).

It seems to me that the only changes needed are:

• update version of ethereumjs-common dependency within ethereumjs-vm after Add petersburg aka constantinople fix support ethereumjs-common#44 is merged and npm package is updated
• update ethereumjs-testing to include changes from Constfix ethereum/tests#582
• update version of ethereumjs-testing dependency within ethereumjs-vm
• update the tests/constantinopleSstoreTest.js file added in EIP 1283 SSTORE #367 to include checks related to forks before and after constantinople

I can do this over the next 36 hours. Do let me know if I am missing or misunderstanding something related to actual code changes needed.

[holgerd77]

Hi Dan, maybe I described this as a bit more complicated in my first post/analysis than it actually is. No, I don't think that your missing something and your description of the changes/updates needed seems to be very much accurate to me. Nevertheless we should handle this task with the necessary care, at the end this is still some fork update.

Some notes:

• Normally I would want to wait on updating ethereumjs-testing for an official release on ethereum/tests to assure that things are officially declared as stable and finalized, I have asked on that here. Since we have some time-critical aspect here as well, it might nevertheless in this case be good to make an exception and do a commit based ethereumjs-testing v1.2.7 release. Will do directly after posting here.
• I will then also update Updated tests to v6.0.0-beta.3 (v1.2.6) #416, add the failing Constantinople tests to the temporary skip lists and then merge here

Can you prepare the PR with the changes necessary to the VM and including the fork switching logic you described? For now this can temporary point to the ethereumjs-common PR branch (or maybe we'll get a release ready short-term, I would nevertheless like to have one proper review on this), which can later be updated by rebase to the final released version.

Regarding the tests: the test file tests/constantinopleSstoreTest.js you mentioned was just introduced by Vinay since there were no official tests at the time of implementation. We probably shouldn't further expand on this and just leave as is and concentrate on the official tests.

For the time of transition (Byzantium (-> Constantinople) -> Petersburg) I would suggest the following here:

• We add a third state test state test command testStatePetersburg to both package.json and .circleci/config.yml (I think CI should be able to still handle this regarding CI run time)
• We should also do the blockchain test run fork selection more explicit by adding a CL param to the testBlockchain command (I wouldn't switch to the named commands here since we only have the capacity to run one version of the blockchain tests anyhow on CI). I would introduce this with a constantinople setting on the ethereumjs-testing update PR on the VM, this can afterward be switched to petersburg

If you go along you can directly include these testing updates in your VM fork logic PR.

Cheers
Holger

[holgerd77]

Some update: also don't take the reviewer choice e.g. on ethereumjs/ethereumjs-common#44 for granted, I normally just choose someone where I hope that the person has some time. You are of course also very much invited (and it is super helpful) to do reviews on PR where there is no direct request.

[holgerd77]

Hi Dan @danjm, ok, have done a new ethereumjs-testing release and merged the constantinople fix tests into the VM #430, you should be able to work with this.

Note that the fork in the tests are (currently) named constantinopleFix, while we'll use petersburg as the more official name (also used e.g. in geth) in the Common class + related code.

There are also changes needed on the supportedHardforks definition and the opts.hardfork JSDoc comment on the VM constructor.

[danjm]

Thanks @holgerd77, I'll get started on this now.

[holgerd77]

Hi Dan, once you got something here can you open a PR? Doesn't need to be ready (then just mark as "WIP", "DO NOT MERGE"), just to have some common ground for further work or discussion.

[danjm]

@holgerd77 Yes, will do. I just put up two PRs ethereumjs/ethereumjs-common#46 and #432 that will be needed before a Constantinople-Petersburg PR can work. They address some breaking changes introduced with ethereumjs-common v1.0.0 (introduced before your recent Constantinople reentrancy PR).

Even with those changes, there is still something broken in ethereumjs-vm when using common v1.0.0. This is the only blocker to my work on the present issue. I hope to have found and resolved the source of this soon. (Edit: fixed)

[danjm]

@holgerd77

#433 is the PR that will resolve this issue in ethereumjs-vm

There are a few PRs up across multiple repos that need to be merged for #433 to work. When I put them altogether in one local ethereumjs-vm repo, including your recent changes to ethereumjs-common and ethereumjs-testing, tests pass. (I ran api, blockchain and state tests locally.)

I believe we should proceed in the following order:

1. Merge Improve api for importing genesisStates, chains and hardforks in depe… ethereumjs-common#46. This is non-essential, but preferred. It allows for a cleaner, and probably less buggy, importing of ethereumjs-common interfaces.
2. Release a new version of ethereumjs-common.
3. Merge Update to ethereumjs-common v1.1.0 ethereumjs-block#64
4. Release a new version of ethereumjs-block
5. Merge Update to ethereumjs-common v1.1.0 ethereumjs-blockchain#86
6. Release a new version of ethereumjs-blockchain
7. Update Updates package.json and code to use latest ethereumjs-common version #432 to include the versions of common, block and blockchain released in steps 2, 4 and 6.
8. Merge Updates package.json and code to use latest ethereumjs-common version #432

Update: PRs linked in 1, 3, 5 have been approved by Whymarrh

[holgerd77]

Hi @danjm, thanks for the clear summary of the process, super helpful! 😄 I am on it to work down the stuff, see comments on the associated repos.

[holgerd77]

Hi @danjm Dan, ok, v1.1.0 of ethereumjs-common just released 😄, you should now be able to use this here as well as for the block and blockchain PRs.

[holgerd77]

Ok, all necessary releases (common, block, blockchain) are out 😄! Can you tie everything together?

Would be cool if we can get a VM release out until Thursday!

Let me know if anything is still missing or unclear.

Cheers
Holger

[danjm]

@holgerd77 The updates to the three library versions (common, block and blockchain) as well as all vm changes are now in #433

Is anything else needed?

[holgerd77]

Closed by #436.