VM Bugfixes

[jochem-brouwer]

This PR fixes two bugs in the VM which appeared when we ran mainnet blocks in #853

They both have to do with Istanbul SSTORE gas calculation: the first fixes that the storage cache is only wiped after a block (if two txs write the same storage, then this slot is cached and there are some gas refunds which you get in the VM which should not happen). The second is checking if the slot has the same value; they can have the same value, but due to wrongly padded Buffers the VM thinks that the values are different. (i.e. the buffers 0x01 and 0x0001 are semantically in the VM the same value, but the Buffers are obviously not equal).

[codecov]

Codecov Report

Merging #870 into master will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Flag	Coverage Δ
#account	92.85% <ø> (ø)
#block	77.37% <ø> (-0.26%)	⬇️
#blockchain	81.10% <ø> (ø)
#common	94.58% <ø> (ø)
#ethash	82.22% <ø> (-1.12%)	⬇️
#tx	93.98% <ø> (-0.14%)	⬇️
#vm	82.24% <100.00%> (+0.05%)	⬆️

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

[jochem-brouwer]

WIP because:

(1) will write unit tests for these situations

(2) have to cleanup this padding stuff

[jochem-brouwer]

Added tests. The cache situation is rather complex, I wanted to find the situation which I found in the mainnet blocks, but it seems to be rather complex. It has to do with the final state not being committed (because if it is commited, then the cache was originally cleared too). If it is reverted the originally cache was not cleared. But this leads to a rather complex situation, so I have only added a simple test.

[jochem-brouwer]

Huh, this is confusing, now the tests are failing 😢 Will fix soon.

[jochem-brouwer]

Tests failing was #729-related again.

[holgerd77]

This looks a lot better than the first version, great, thanks Jochem for the update. And actually really important bug fixes since especially these "only occur under very special conditions" bugs are really hard to track but make the VM "feeling" less reliable since they WILL get triggered unpredictably every now and then.

So: really cool. 😄

Will merge and hopefully integrate correctly into #853

[holgerd77]

Thanks Jochem, I am really learning a lot here on how to set up opcode based test cases (+ the associated gas cost analysis), really great!

[holgerd77]

@jochem-brouwer update: ah, just realize that you already removed these fixes from #853, thanks for doing that! 😄 Then I will just do a "normal" rebase on this branch and push-force to update to latest master state.

[holgerd77]

Update: just realizing that some API tests are still failing on this branch. A bit tricky, this was not triggered since not all API tests were running here. This is due to the test:API script not single quoting the wildcard folder passed to tape, discovered this in parallel when working on #869. So if you add and run npx tape -r ts-node/register --stack-size=1500 './tests/api/**/*.js', these failing tests get triggered (fixed this in #853, but this might also be a case for extraction).

@jochem-brouwer can you please have a look into this and open a fixing PR? It seems to me that some of the changes here might have had side effects which were not visible/triggered before.

[holgerd77]

Hi @jochem-brouwer, can confirm that the failing API tests have been fixed along #872, so no worries here! 😄

[jochem-brouwer]

@holgerd77 great thanks for letting me know! Is there also a CI fix in place somewhere which ensures all tests are being ran?

[holgerd77]

@jochem-brouwer yes, that has been fixed along one of my last PRs (where I actually wasn't aware of myself, just discovered that this change is "in" and had to look this up myself 😅 )