Add state tests for Istanbul #607
Conversation
|
The tester was instantiating |
| "testBuildIntegrity": "npm run build:dist && node ./tests/tester -s --dist --test='stackOverflow'", | ||
| "testBlockchain": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --fork='Petersburg' --dist --excludeDir='GeneralStateTests'", | ||
| "testBlockchain": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --fork='Istanbul' --dist --excludeDir='GeneralStateTests'", | ||
| "testBlockchainPetersburg": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --fork='Petersburg' --dist --excludeDir='GeneralStateTests'", |
There was a problem hiding this comment.
I'm curious about this line. Weren't we testing some of the HF in the CI?
There was a problem hiding this comment.
We are always only testing the latest or eventually the latest two HFs on CI, otherwise this would blow CI time (this nevertheless improved recently and e.g. allowed to add the blockchain tests beneath the state tests at all, before only state tests were running).
There was a problem hiding this comment.
Got it, thanks for the reply.
There was a problem hiding this comment.
On second thought, maybe we can cron a daily full test run on master using https://circleci.com/docs/2.0/configuration-reference/#schedule
I use the travis equivalent for Buidler, and lets me catch errors introduced by dependencies asap.
There was a problem hiding this comment.
So we're running the state tests for all HFs from Byzantium, but the blockchain tests were being run only for the latest HF because of their runtime, which was improved in #536. So I guess now we could run the blockchain tests for maybe 2-3 HFs.
Scheduling cron jobs is a good idea, I like it. We could also run run slow tests (#472) as a cron job.
The VM's testing infrastructure could generally use some improvements.
* Store refund counter in EVM * Fix linting error
|
Some Istanbul tests included accounts in the |
|
#612 had caused some of the EIP-2200 API tests to break because they were checking against So after |
|
I just left a comment about the breaking change in the other PR. I'm not against the change itself, but I think there's probably some people depending con that field, so we shouldn't change it without releasing a major version. |
I thought about this some more and I think we can keep it quasi-backwards-compatible (not quite). The difference would be that The reason is that before EIP-2200 (excluding constantinople), Now however (after EIP-2200) I don't have a strong preference for either, and could change to the second option. |
|
Haven't confirmed yet, but I think the reasons many blockchain tests are failing is that ^^ Another argument for a monorepo :) |
|
I have to think a little more about the
@davidmurdoch would this be a problem for Ganache? |
|
Ganache's gas estimations rely on the Probably not helpful, but here is some background details on why the property was introduced: #283 |
|
I've been playing with TAPE and its reporting methods, EVMDIS and other tools, to make a better analysis of these failing tests. Will update here as I go.
RevertPrecompiledTouchExactOOG.json
RevertPrecompiledTouch_storage.json
|
|
This pull request introduces 1 alert when merging c3e1ccf into 00d8fe3 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
c3e1ccf fixes #608, and now all state tests are passing 😄
For context on the issue of deleting precompiles when a revert happens see EIP-716 and ethereum/aleth#5664.
| @@ -163,10 +161,6 @@ export default class EEI { | |||
| * input data passed with the message call instruction or transaction. | |||
| */ | |||
| getCallDataSize(): BN { | |||
| if (this._env.callData.length === 1 && this._env.callData[0] === 0) { | |||
There was a problem hiding this comment.
Some tests were using CALLDATASIZE as a way of determining the address to call. They were failing for the case were calldata was Buffer<00> (CALLDATASIZE returned 0 instead of 1).
I'm not sure why this condition was here? any ideas?
There was a problem hiding this comment.
This is really weird. I checked in pyevm, and it doesn't have this logic either.
|
|
||
| const err = result.execResult.exceptionError | ||
| if (err) { | ||
| result.execResult.logs = [] | ||
| await this._state.revert() | ||
| if (message.isCompiled) { |
There was a problem hiding this comment.
Here we were re-touching any precompile in case of OOG.
| // See [EIP-716](https://github.com/ethereum/EIPs/issues/716) for context. | ||
| // The RIPEMD precompile has to remain *touched* even when the call reverts, | ||
| // and be considered for deletion. | ||
| if (this._touched.has(ripemdPrecompileAddress)) { |
There was a problem hiding this comment.
I moved the re-touching of precompiles in case of OOG here (not a strong preference, I could re-move it to evm.ts). It is also limited to the precompile at 0x03.
| this._result.gasRefund = new BN(0) | ||
| this._evm._refund.isub(amount) | ||
| if (this._evm._refund.ltn(0)) { | ||
| this._evm._refund = new BN(0) | ||
| trap(ERROR.REFUND_EXHAUSTED) | ||
| } |
There was a problem hiding this comment.
Maybe we can do this._result.gasRefund = this._evm._refund after modifying it. It clutters the code a little, but it avoids a breaking change.
As David, said Ganache only depends on gasRefund at the end of every tx, so this would work.
There was a problem hiding this comment.
I re-added gasRefund to ExecResult, by copying EVM._refund at the end of message execution in EVM. This should work for the Ganache use-case. I think we should be backwards-compatible now, at least for the most part.
|
This pull request introduces 1 alert when merging b26f15c into 00d8fe3 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging b9ebe69 into 00d8fe3 - view on LGTM.com new alerts:
|
|
Strange. Circleci is failing with this error on It's as is if the Update: it was failing to checkout to the |
|
This pull request introduces 1 alert when merging a29de25 into 00d8fe3 - view on LGTM.com new alerts:
|
|
Ok, we are stumbling over our own improvements: # should contain values of hardcoded bitvector
/home/circleci/project/ethereumjs-vm/node_modules/ethereumjs-util/dist/bytes.js:79
throw new Error("Cannot convert string to buffer. toBuffer only supports 0x-prefixed hex strings and this string was given: " + v);
^
Error: Cannot convert string to buffer. toBuffer only supports 0x-prefixed hex strings and this string was given: value 1
at Object.exports.toBuffer (/home/circleci/project/ethereumjs-vm/node_modules/ethereumjs-util/dist/bytes.js:79:23)
at Test.t.test (/home/circleci/project/ethereumjs-vm/tests/api/bloom.js:25:27)
at Test.bound [as _cb] (/home/circleci/project/ethereumjs-vm/node_modules/tape/lib/test.js:66:32)
at Test.run (/home/circleci/project/ethereumjs-vm/node_modules/tape/lib/test.js:85:10)
at Test.bound [as run] (/home/circleci/project/ethereumjs-vm/node_modules/tape/lib/test.js:66:32)
at Test._end (/home/circleci/project/ethereumjs-vm/node_modules/tape/lib/test.js:154:11)
at Test.bound [as _end] (/home/circleci/project/ethereumjs-vm/node_modules/tape/lib/test.js:66:32)
at Test.<anonymous> (/home/circleci/project/ethereumjs-vm/node_modules/tape/lib/test.js:153:40)
at emitNone (events.js:106:13)
at Test.emit (events.js:208:7)
Otherwise, great! 😀 Blockchain test passing as well! On the release question: would it be enough to release this as |
Interestingly #617 is also failing with the same error, hmm 🤔
I'll try to use Pato's approach for |
|
This pull request introduces 1 alert when merging 18b34f9 into 00d8fe3 - view on LGTM.com new alerts:
|
I had to fix some tests in The problem is that it's hard to detect when a change like this would affect other packages before releasing it. I know I sound like a broken record, but this is why I'm a big fan of monorepos 😅 |
|
This pull request introduces 1 alert when merging 57d7483 into 00d8fe3 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 970ddbf into 00d8fe3 - view on LGTM.com new alerts:
|
|
The PR is ready for review 😄
For this error I made the Also updated I think we might need to do something about this. Each of Tx, Block, Blockchain and VM accept a Common (or chain and hardfork) object and mismatch could easily happen, specially since |
holgerd77
changed the title
This points to ethereumjs/ethereumjs-testing#42 for upstream state tests. The dependency has to be fixed after ethereumjs-testing releases the istanbul tests.
Due to the re-structuring of the tests directories, we have to configure tester where to look for tests depending on the HF (tests for HFs prior to Istanbul are in
LegacyTests).A lot of tests are failing, even for Petersburg state tests. I'll now focus on figuring out what the issues are. @evertonfraga offered to help with resolving them.
(also closes #608 and #611)