Fix Constantinople tests

[evertonfraga]

Thanks for the contribution, @jochem-brouwer!

Closes #928 and fixes nightly builds.

[codecov]

Codecov Report

Merging #931 into master will not change coverage.
The diff coverage is n/a.

Flag	Coverage Δ
#block	76.06% <ø> (ø)
#blockchain	80.33% <ø> (ø)
#common	92.03% <ø> (ø)
#ethash	82.08% <ø> (ø)
#tx	88.18% <ø> (ø)
#vm	87.17% <ø> (ø)

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

[jochem-brouwer]

Can we run the Constantinople tests in this PR just to (double) make sure that my claim it is fixed is actually true? 😄

[evertonfraga]

@jochem-brouwer sure! doing it now

[jochem-brouwer]

Heh. Looks like other tests are now failing 😅 I could take a look at this tomorrow.

[cgewecke]

Ah yes this make sense, sorry and thanks! 🙂

Perhaps the misplaced stuff could also go in a function in opcodes/utils as updateSstoreGas?

      updateSstoreGas(runState, found, setLengthLeftStorage(value))
      updateSstoreGasEIP1283(runState, found, setLengthLeftStorage(value))
      updateSstoreGasEIP2200(runState, found, setLengthLeftStorage(value), keyBuf)

[jochem-brouwer]

Yep great suggestion! I will also move the HF logic inside the SSTORE, because otherwise it seems that it updates SSTORE gas 3 times (which is not the case)

[cgewecke]

Oh nice, yes that's a lot clearer now 💯

[ryanio]

Great PR :) It looks like all the uses of adjustSstoreGasEIP2929 are being wrapped in a new BN, should we just alter the function to return a BN and simplify some of the syntax?

[jochem-brouwer]

Can you point to these lines @ryanio ?

These new BN things are very misleading, I never understood why this was done in the first place as especially the new keyword implies that you are actually creating a new (i.e. independent) object.

However...

> a = new BN(1)
<BN: 1>
> b = new BN(a)
<BN: 1>
> b.iaddn(1)
<BN: 2>
> a
<BN: 2>
> b
<BN: 2>

[ryanio]

These new BN things are very misleading, I never understood why this was done in the first place as especially the new keyword implies that you are actually creating a new (i.e. independent) object.

Yeah I find that confusing as well, that clone() is actually needed to create a new independent object. I would also expect it to be part of the new BN initialization.

re: adjustSstoreGasEIP2929, just look at all its uses here in the PR, it's returning a number but always needing to be wrapped as a BN, so my suggestion is to make the return value a BN so we can reduce some of these crazy amount of new BNs.

Most of it is common.param though, which we can possibly take in another PR, as you suggest in #914, would be pretty great to have that return a BN I think that would possibly reduce 100s of extra new BNs. I guess just being nit picky here, no stopper 😆

[jochem-brouwer]

Ah I see! Just a note here these new BNs are not introduced here, might look like it due to the diff but it is merely copying the original logic around.

I think it would be great to do this in a seperate PR, possibly together with #914

[ryanio]

ah right ok, yeah sounds like something good for future work then. This LGTM then!

[holgerd77]

@jochem-brouwer Great stuff here! 😄

Could you please update this post-merge with a proper title and a short description on what was broken respectively has been fixed for the release notes? Thanks!

[holgerd77]

(also can please everyone try hard to internalize our new process of upgrading the release notes along PRs 😜 and then hopefully directly add to the release notes on PRs and also mention missing release notes on PR reviews? Seems this is still forgotten 80%+ of the times 😋 🎃 )