Adria0/fix/tests #11597

adria0 · 2020-04-03T07:06:38Z

Issue for this PR is https://github.com/openethereum/openethereum/issues/11647

Fixing/syncing #11085 [json-tests] run all tests (or justify why not)
with the last available ethereum/tests@5841af6

dvdplm

Good stuff! LGTM but let's wait for CI.

ethcore/src/json_tests/test_common.rs

ethcore/src/test_helpers/evm_test_client.rs

evmbin/src/main.rs

Co-Authored-By: David <dvdplm@gmail.com>

…o adria0/fix/tests

…into adria0/fix/tests

ethcore/machine/src/executive.rs

ethcore/src/json_tests/chain.rs

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

ethcore/machine/src/executive.rs

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

rpc/src/v1/tests/eth.rs

ordian · 2020-05-04T08:55:26Z

ethcore/machine/src/executive.rs

+						// EIP161 - reverted calls cancels state trie cleaning
+						if initial_builtin_balance.is_zero() {
+							let prune = UNPRUNABLE_PRECOMPILE_ADDRESS
+								.map_or(true, |addr| addr != params.code_address);
+							if prune {
+								substate.touched.remove(&params.code_address);
+							} 
+						}


just to make sure I understand this correctly: for tests this will remove all but ripemd builtints with zero balance and for production this revert all zero-balanced builtins (although in practice all builtins have balance: 1, maybe there are some exceptions https://github.com/openethereum/openethereum/pull/11482/files#r378702588, this needs to be double checked)

That's a great observation, classic and foundation have balance == 0 more might have it too.

for tests this will remove all but ripemd builtints with zero balance

we need to be able to reproduce the RIPEMD removal at block #2686351 in tests

for production this revert all zero-balanced builtins

afaik, all precompiles has balance: 1 due the EIP161 rule. for instance, when you create a new clique genesis with geth, all precompiles balances are set to 1

maybe there are some exceptions

this issue happened due that both geth and parity allowed the removal of 0x3 account for different reasons, and nobody noticed about it after some time, so core devs decided to introduce this "irregular state transition".

I agree with you that this change could affect to PoA chains with the same scenario:

they initially specified the precompile in the trie with balance=0

they called a precompile without enough gas and the precompile reverted and was removed from the trie

Maybe we can add there a eip161_touch_prunning_bug, but this is really ugly 😱

Moving everything to chain definitions is another option,

eip161_unprunable_precompiles: list of precompile addresses that cannot be pruned if touched with zero balance in a reverted call.

we have to add this flag to the testnet with 0x3, and also allows fixing existing PoA chains. I think that we need to generate some kind of an explicit warn! log there to help to identity the problem.

@ordian @niklasad1,

I see these options now:

(a) Fix EIP161 only in tests: in real deployments, this is "tactically fixed" by setting balances to >=1 in precompile accounts.

(b) Fix EIP161 at all

(b.1) add a chain option eip161_unprunable_precompiles=addr, addr,... and define it in mainnet chains with 0x3

(b.2) add a chain option eip161_touch_prunning_bug, a flag that could be activated by any existing PoA chains that removed some precompile accounts

(c) Introduce generic irregular state transitions (I do not like this)

Any other idea? Thoughts?

I would go for option (a) as the easiest one, but we would need to ensure (somehow via CI and document it enough) that each precompile has balance >= 1 indeed. The problem with changing balance for existing chains is that it's a breaking change (see e.g. #11108).

@ordian, implemented (a) in this PR, commit openethereum/openethereum@04dc267

dvdplm

Overall this looks good (great actually). I have a doubt about the "legacy" tests: unless we have a compelling reason for keeping them I am not sure they are worth keeping. Do you think you can expand the description with a motivation why?

ethcore/res/ethereum/test-specs/byzantium_to_constantinoplefixat5_test.json

ethcore/res/ethereum/test-specs/eip158_to_byzantiumat5_test.json

ethcore/machine/src/executive.rs

json/src/test_helpers/skip.rs

json/src/test_helpers/blockchain/mod.rs

ethcore/src/json_tests/state.rs

ethcore/src/json_tests/chain.rs

ethcore/machine/src/executive.rs

dvdplm · 2020-05-04T09:59:37Z

I know this isn't a fair comparison but I ran $ time cargo test --all --release --features=json-tests > /dev/null 2>&1 on both this branch and master and got:

Master:

real	3m0.105s
user	4m27.378s
sys	0m7.523s

This branch:

real	4m17.238s
user	10m55.691s
sys	0m23.453s

(This does not include build times; I ran both commands twice.)

Co-authored-by: David <dvdplm@gmail.com>

adria0 · 2020-05-04T14:13:36Z

Absolutely aware of it. It's horrible how compilation+test times are growing :(

We need a better strategy than adding and adding more tests. Removing legacy ones for the "normal" builds is an option.

ordian · 2020-05-04T15:36:42Z

please merge with master (just merged #11675 which updated the submodule)

…ria0/fix/tests

dvdplm

Let's get this merged. Awesome job @adria0 🍰

niklasad1

Very good job looks good modulo some style nits in ethcore/src/json_tests/chain.rs.

Make sure that your editor removes trailing whitespaces
Function calls/function definitions should have a space between for example fn foo(a: usize, b: usize)

ordian

Great job!

* master: Adria0/fix/tests (#11597)

* master: Implement Simple Subroutines for the EVM (EIP 2315) (#11629) Adria0/fix/tests (#11597)

adria0 added 3 commits April 3, 2020 08:41

wip

1cd7a1c

remove dvdplm comment

f001424

tab/space stuff

55f49ed

dvdplm reviewed Apr 3, 2020

View reviewed changes

ethcore/src/json_tests/test_common.rs Outdated Show resolved Hide resolved

ethcore/src/test_helpers/evm_test_client.rs Outdated Show resolved Hide resolved

evmbin/src/main.rs Show resolved Hide resolved

adria0 and others added 5 commits April 3, 2020 11:18

Update ethcore/src/json_tests/test_common.rs

c6bc6a8

Co-Authored-By: David <dvdplm@gmail.com>

Update ethcore/src/test_helpers/evm_test_client.rs

d027d38

Co-Authored-By: David <dvdplm@gmail.com>

add eip1234 transition to byz_to_conAt5

227c1be

Merge branch 'adria0/fix/tests' of github.com:adria0/openethereum int…

535db16

…o adria0/fix/tests

Add pending v7 tests

8bfba81

adria0 mentioned this pull request Apr 9, 2020

'ByzantiumToConstantinopleFixAt5' chain spec for json tests #10461

Closed

adria0 added 2 commits April 9, 2020 10:06

panic tests when chainspec is unimplemented

ed33e53

verify post_state in BlockchainTests

ff6c415

adria0 mentioned this pull request Apr 22, 2020

Fix tests #11647

Closed

6 tasks

adria0 added 3 commits April 25, 2020 00:50

fix stRevertTests

74eabcc

Merge branch 'master' of https://github.com/openethereum/openethereum …

05e7f78

…into adria0/fix/tests

update to last eth/test develop

6575ebc

adria0 force-pushed the adria0/fix/tests branch from e0cf12a to 6575ebc Compare April 28, 2020 07:58

ordian mentioned this pull request Apr 30, 2020

Submodule commit for ethereum/tests only exists in a fork #11668

Closed

ordian linked an issue Apr 30, 2020 that may be closed by this pull request

Submodule commit for ethereum/tests only exists in a fork #11668

Closed

adria0 added 2 commits May 1, 2020 09:56

update to latest eth/test develop b9f5fe3

89568da

Remove unused H160 compliation warning

c5e53c3

adria0 marked this pull request as ready for review May 4, 2020 07:29

adria0 requested review from vorot93 and niklasad1 May 4, 2020 07:30

adria0 requested a review from dvdplm May 4, 2020 07:30

niklasad1 reviewed May 4, 2020

View reviewed changes

ethcore/machine/src/executive.rs Show resolved Hide resolved

niklasad1 reviewed May 4, 2020

View reviewed changes

ethcore/src/json_tests/chain.rs Outdated Show resolved Hide resolved

Update ethcore/src/json_tests/chain.rs

8dac976

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

niklasad1 reviewed May 4, 2020

View reviewed changes

ethcore/machine/src/executive.rs Outdated Show resolved Hide resolved

Update ethcore/machine/src/executive.rs

8a8abb1

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

adria0 commented May 4, 2020

View reviewed changes

rpc/src/v1/tests/eth.rs Show resolved Hide resolved

ordian reviewed May 4, 2020

View reviewed changes

dvdplm reviewed May 4, 2020

View reviewed changes

Update ethcore/src/json_tests/chain.rs

c1bb0ea

Co-authored-by: David <dvdplm@gmail.com>

adria0 added 3 commits May 4, 2020 18:52

Fix comments and test features

78e358d

Merge branch 'master' of github.com:openethereum/openethereum into ad…

620c354

…ria0/fix/tests

EIP161 precompile balances precondition

04dc267

adria0 force-pushed the adria0/fix/tests branch from ff16ee4 to 04dc267 Compare May 13, 2020 21:51

adria0 requested review from dvdplm, niklasad1 and ordian May 14, 2020 06:55

dvdplm approved these changes May 14, 2020

View reviewed changes

niklasad1 approved these changes May 14, 2020

View reviewed changes

Fix style nits

db330f3

ordian approved these changes May 14, 2020

View reviewed changes

adria0 merged commit 1ace3c7 into openethereum:master May 14, 2020

ordian added the A9-buythatmanabeer 🍻 Pull request is reviewed well and worth buying the author a beer. label May 14, 2020

ordian added a commit that referenced this pull request May 14, 2020

Merge branch 'master' into ao-test-whether-ci-permissions-are-fixed

f4def98

* master: Adria0/fix/tests (#11597)

dvdplm added a commit that referenced this pull request May 25, 2020

Merge branch 'master' into dp/chore/upgrade-to-rocksdb-0.14

9845679

* master: Implement Simple Subroutines for the EVM (EIP 2315) (#11629) Adria0/fix/tests (#11597)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adria0/fix/tests #11597

Adria0/fix/tests #11597

adria0 commented Apr 3, 2020 •

edited

Loading

dvdplm left a comment

ordian May 4, 2020

niklasad1 May 4, 2020

adria0 May 4, 2020

adria0 May 4, 2020 •

edited

Loading

adria0 May 12, 2020

ordian May 12, 2020

adria0 May 13, 2020 •

edited

Loading

dvdplm left a comment

dvdplm commented May 4, 2020

adria0 commented May 4, 2020

ordian commented May 4, 2020

dvdplm left a comment

niklasad1 left a comment •

edited

Loading

ordian left a comment

Adria0/fix/tests #11597

Adria0/fix/tests #11597

Conversation

adria0 commented Apr 3, 2020 • edited Loading

dvdplm left a comment

Choose a reason for hiding this comment

ordian May 4, 2020

Choose a reason for hiding this comment

niklasad1 May 4, 2020

Choose a reason for hiding this comment

adria0 May 4, 2020

Choose a reason for hiding this comment

adria0 May 4, 2020 • edited Loading

Choose a reason for hiding this comment

adria0 May 12, 2020

Choose a reason for hiding this comment

ordian May 12, 2020

Choose a reason for hiding this comment

adria0 May 13, 2020 • edited Loading

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

dvdplm commented May 4, 2020

adria0 commented May 4, 2020

ordian commented May 4, 2020

dvdplm left a comment

Choose a reason for hiding this comment

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

adria0 commented Apr 3, 2020 •

edited

Loading

adria0 May 4, 2020 •

edited

Loading

adria0 May 13, 2020 •

edited

Loading

niklasad1 left a comment •

edited

Loading