new ethereum consensus tests #10923

debris · 2019-07-28T16:27:27Z

ref #10908

discussion explaining changes I made: ethereum/aleth#5664

my limited knowledge of current state of executive and how tests are loaded doesn't allow me to fix all failing tests

tests which are still failing

RevertPrecompiledTouchExactOOG_d31g1v0_Byzantium
RevertPrecompiledTouchExactOOG_d31g1v0_Constantinople
RevertPrecompiledTouchExactOOG_d31g1v0_ConstantinopleFix
RevertPrecompiledTouchExactOOG_d7g1v0_Byzantium
RevertPrecompiledTouchExactOOG_d7g1v0_Constantinople
RevertPrecompiledTouchExactOOG_d7g1v0_ConstantinopleFix

my questions:

what is "network": "EIP158" and why do we load it from json test file eip161_test.json
how is this network different to byzantium and constantinople

cc @sorpaas @tomusdrw

debris · 2019-07-28T16:28:09Z

ethcore/src/executive.rs

@@ -346,6 +346,10 @@ impl<'a> CallCreateExecutive<'a> {
 				| Err(vm::Error::OutOfBounds)
 				| Err(vm::Error::Reverted)
 				| Ok(FinalizationResult { apply_state: false, .. }) => {
+					let ripemd = Address::from_low_u64_be(3);


If we agree that this is a right place for this 'fix', I'll add comments and move this value to the constant

sorpaas · 2019-07-28T22:41:48Z

@debris "EIP158" refers to the Spurious Dragon hard fork (http://eips.ethereum.org/EIPS/eip-607).

sorpaas

I wonder, if this is something that only affects tests. Why not just put it in some place where it's under #[cfg(test)]? I don't feel like putting it into production code is a good choice. evm is also used by crate libraries and other networks. I worry that this hard-coded value may break some of their guarantees.

tomusdrw

Looks good, but like Wei, I'm not convinced about the location of the code. Note that you can create any custom chainspec files with whatever accounts you want and assuming that ripemd is at address 3 for all networks seems a bit off too me.

If there is no way to put this code in a place that is only run for json-tests (note we probably need to have the same logic for evmbin too, since it's used for cross-client fuzzing), we should guard this condition with network ids to make sure it only runs on specific networks (as an alterntative we could consider a chainspec flag to disable this behaviour to avoid breaking consensus).

debris · 2019-07-29T10:22:30Z

Looks good, but like Wei, I'm not convinced about the location of the code.

This is also very confusing to me. Why should it be placed in [cfg(test)] if this change is required by _Byzantium and _Constaninople tests? Why are those tests altered/added now? If they fail, does it imply that we are consensus issue between parity and geth? If it does not imply that, why do we even need them? Are they named correctly? <- just my thoughts, would be good if someone helped me understand the problem deeper

ordian · 2019-07-29T10:29:56Z

This is related: https://github.com/ethereum/go-ethereum/pull/3341/files#r89548318
EDIT: here is more context: ethereum/EIPs#716

debris · 2019-07-29T12:46:29Z

This is related: https://github.com/ethereum/go-ethereum/pull/3341/files#r89548318

I saw that. If above is true, why are the tests failing and why do we need to implement anything?

dvdplm · 2019-08-27T10:04:05Z

@debris what is the status of this? Can we move forward on it?

This PR reverts the controversial changes of the previous PR and skips the failing tests. Maybe I misunderstand the suggested workaround of putting the fix under `#[cfg(test)]` but it seems odd to run different code in production than we run in tests. Instead here I suggest we skip the failing tests with the argument that we do not wish to fix this issue (at least not at this time) because it does not affect us. If I am wrong, and I likely am, I look forward to hearing why and what a better approach to updating the state tests is. Branched off #10923 ref #10908

* new ethereum consensus tests, #10908 * Update JSON tests to 725dbc73a This PR reverts the controversial changes of the previous PR and skips the failing tests. Maybe I misunderstand the suggested workaround of putting the fix under `#[cfg(test)]` but it seems odd to run different code in production than we run in tests. Instead here I suggest we skip the failing tests with the argument that we do not wish to fix this issue (at least not at this time) because it does not affect us. If I am wrong, and I likely am, I look forward to hearing why and what a better approach to updating the state tests is. Branched off #10923 ref #10908 * Update json test commit to 1dc9d20e97165708f7db0bbf2d1a87a6b4285827 * Fail with error message * Handle missing r, s, v params in json tests Light cleanup of json test runner * Include the path to the test file * Handle new `postState` format: string or map Sort out tests Missing docs * WIP * Include test-helpers from ethjson * Sort out new paths * Remove dead code * Fix warnings stemming from code called only from macros Skip failing tests in stRevert/ and stTransactionTest/ (too course a filter!) Docs and light touch refactorings for readability * Skip all failing tests * Document the single-test-skipping madness * Update tests to latest commit on the `develop` branch * Rename test skipping types to reflect actual purpose * Switch to skipping individual tests in currents.json Add some logging to help debug skipping * Fix rpc test by curve fitting to new json test source file * Add refs to all issues for fixing failing&skipped json tests * Sort out the need for Clone for tests * [json-tests] populate state from genesis pod state (#11083) * [json-tests] populate state from genesis pod state * [json-tests] #11075 is resolved as well * [json-tests] #11076 hopefully too * [json-tests] #11077 🎉 * [json-tests] fix trailing comma * Update ethcore/src/json_tests/chain.rs Co-Authored-By: Andronik Ordian <[email protected]> * Add issue numbers to TODOs * Apply @ordians fix for wrong state_root * Warn on invalid RLP * Remove the `ci-skip-tests` feature

* ethcore/res: activate Istanbul on Ropsten, Görli, Rinkeby, Kovan (#11068) * ethcore/res: activate Istanbul on Ropsten block 6485846 * ethcore/res: activate Istanbul on Goerli block 1561651 * ethcore/res: use hex values for Istanbul specs * ethcore/res: fix trailing comma * ethcore/res: be pedantic about EIP-1283 in Petersburg and Istanbul test specs * ethcore/res: activate Istanbul on Rinkeby block 5435345 * ethcore/res: activate Istanbul on Kovan block 14111141 * ethcore/res: fix kovan istanbul number to 0xd751a5 * cleanup json crate (#11027) * [json]: cleanup write something here.... * nit: commit new/moved files * nit: remove needless features * nits * fix(grumbles): use explicit import `DifficultyTest` * fix(grumbles): remove needless type hints * fix(grumble): docs `from -> used by` Co-Authored-By: David <[email protected]> * fix(grumbles): use explicit `imports` * fix(grumble): merge `tx` and `tx_with_signing_info` * fix(grumbles): resolve introduced `TODO's` * [json-spec] make blake2 pricing spec more readable (#11034) * [json-spec] make blake2 pricing spec more readable * [ethcore] fix compilation * Update JSON tests to d4f86ecf4aa7c (#11054) * new ethereum consensus tests, #10908 * Update JSON tests to 725dbc73a This PR reverts the controversial changes of the previous PR and skips the failing tests. Maybe I misunderstand the suggested workaround of putting the fix under `#[cfg(test)]` but it seems odd to run different code in production than we run in tests. Instead here I suggest we skip the failing tests with the argument that we do not wish to fix this issue (at least not at this time) because it does not affect us. If I am wrong, and I likely am, I look forward to hearing why and what a better approach to updating the state tests is. Branched off #10923 ref #10908 * Update json test commit to 1dc9d20e97165708f7db0bbf2d1a87a6b4285827 * Fail with error message * Handle missing r, s, v params in json tests Light cleanup of json test runner * Include the path to the test file * Handle new `postState` format: string or map Sort out tests Missing docs * WIP * Include test-helpers from ethjson * Sort out new paths * Remove dead code * Fix warnings stemming from code called only from macros Skip failing tests in stRevert/ and stTransactionTest/ (too course a filter!) Docs and light touch refactorings for readability * Skip all failing tests * Document the single-test-skipping madness * Update tests to latest commit on the `develop` branch * Rename test skipping types to reflect actual purpose * Switch to skipping individual tests in currents.json Add some logging to help debug skipping * Fix rpc test by curve fitting to new json test source file * Add refs to all issues for fixing failing&skipped json tests * Sort out the need for Clone for tests * [json-tests] populate state from genesis pod state (#11083) * [json-tests] populate state from genesis pod state * [json-tests] #11075 is resolved as well * [json-tests] #11076 hopefully too * [json-tests] #11077 🎉 * [json-tests] fix trailing comma * Update ethcore/src/json_tests/chain.rs Co-Authored-By: Andronik Ordian <[email protected]> * Add issue numbers to TODOs * Apply @ordians fix for wrong state_root * Warn on invalid RLP * Remove the `ci-skip-tests` feature

dvdplm · 2019-10-09T11:02:04Z

Closing as stale.

new ethereum consensus tests, #10908

b745fa3

debris added A0-pleasereview 🤓 Pull request needs code review. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 28, 2019

debris requested review from tomusdrw and sorpaas July 28, 2019 16:27

debris commented Jul 28, 2019

View reviewed changes

sorpaas reviewed Jul 28, 2019

View reviewed changes

tomusdrw reviewed Jul 29, 2019

View reviewed changes

This was referenced Jul 29, 2019

Update json tests #10400

Closed

Add missing test declaration for stExtCodeHash #10367

Closed

dvdplm mentioned this pull request Sep 13, 2019

Update JSON tests to d4f86ecf4aa7c #11054

Merged

8 tasks

dvdplm closed this Oct 9, 2019

ordian deleted the new-ethereum-consensus-tests branch October 16, 2019 10:02

adria0 mentioned this pull request Apr 24, 2020

Fix tests #11647

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new ethereum consensus tests #10923

new ethereum consensus tests #10923

debris commented Jul 28, 2019 •

edited

Loading

debris Jul 28, 2019

sorpaas commented Jul 28, 2019

sorpaas left a comment

tomusdrw left a comment

debris commented Jul 29, 2019

ordian commented Jul 29, 2019 •

edited

Loading

debris commented Jul 29, 2019 •

edited

Loading

dvdplm commented Aug 27, 2019

dvdplm commented Oct 9, 2019

new ethereum consensus tests #10923

new ethereum consensus tests #10923

Conversation

debris commented Jul 28, 2019 • edited Loading

debris Jul 28, 2019

Choose a reason for hiding this comment

sorpaas commented Jul 28, 2019

sorpaas left a comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

debris commented Jul 29, 2019

ordian commented Jul 29, 2019 • edited Loading

debris commented Jul 29, 2019 • edited Loading

dvdplm commented Aug 27, 2019

dvdplm commented Oct 9, 2019

debris commented Jul 28, 2019 •

edited

Loading

ordian commented Jul 29, 2019 •

edited

Loading

debris commented Jul 29, 2019 •

edited

Loading