New merge tests by zilm13 · Pull Request #2598 · ethereum/consensus-specs

zilm13 · 2021-09-14T08:53:19Z

Added some merge tests, all I was able to discover. Not a good test creator though
Covering:

more bad cases with process_execution_payload
unit tests on is_merge_complete, is_merge_block, is_execution_enabled
testing out compute_terminal_total_difficulty's max option
is_valid_terminal_pow_block tests and process_merge_execution_payload

The latter one is a new method combining all checks for terminal block in on_block handler. Appropriate changes to spec was made too. Also fixed minor type misalign in compute_terminal_total_difficulty.
@mkalinin @protolambda Please, take a look

…n function

…s generator test

zilm13 · 2021-09-15T15:28:53Z

I see some options for client tests, researching it, so NO MERGE at the moment, I think we could add some new test generators

…ests without overflow

djrtwo · 2021-09-22T19:32:23Z

Would you mind rebasing to dev to handle the conflicts?

djrtwo

Nice! a handful of comments.

Needs a rebase on dev due to the removal of dynamic TTD. Will do another review after that

specs/merge/fork-choice.md

tests/core/pyspec/eth2spec/test/helpers/block.py

tests/core/pyspec/eth2spec/test/helpers/execution_payload.py

tests/core/pyspec/eth2spec/test/merge/block_processing/test_process_execution_payload.py

djrtwo · 2021-09-22T19:44:03Z

tests/core/pyspec/eth2spec/test/merge/block_processing/test_process_execution_payload.py

+
+    # execution payload
+    execution_payload = build_empty_execution_payload(spec, state)
+    execution_payload.gas_limit = uint64(0)


Now that I look at it, some of the gas_limit validations in is_valid_gas_limit can be validated even on the first_payload. Another option would be to call is_valid_gas_limit not matter what and pass in None or the prev exection header and premise some of the checks on if there was a prev execution header or not.

OR we can leave as is

OR we can remove because the execution-layer will perform these checks anyway.

cc: @mkalinin

oh and marginal preference to turn is_valid_gas_limit into a validate_gas_limit which actually has the line by line assertions. This makes debugging much easier than calling assert on a multi-line predicate.

Not relevant for this PR but want to drop it here while I'm thinking about it

@djrtwo from my experience any logic duplication leads to bugs when we change it someday in one place and forget about another. So I'd vote for removing it if there are no security threats by letting everything up to execution layer decision

The intention was to put all the validations to the consensus layer except for verification that require execution state accesses like transaction validation and state root matching after the execution. The rationale behind this intention is to verify as much as we can before communicating with the execution client, also, this is gonna be a requirement for the statelessness introduction in the future, none of this arguments is that strong though. For now we decided to avoid uint256 math that is required to validate base_fee_per_gas and it might make sense to remove is_valid_gas_limit at all.

Despite the above, I like the idea of turning is_valid_gas_limit to validate_gas_limit.

tests/core/pyspec/eth2spec/test/merge/block_processing/test_process_execution_payload.py

tests/core/pyspec/eth2spec/test/merge/fork_choice/test_terminal_validity.py

tests/core/pyspec/eth2spec/test/merge/unit/test_unit.py

hwwhww

Great work! 👍

Some minor suggestions on pyspec conventions. I didn't review test_terminal_validity.py in detail since it will change after rebasing.

specs/merge/fork-choice.md

tests/core/pyspec/eth2spec/test/merge/fork_choice/test_terminal_validity.py

tests/core/pyspec/eth2spec/test/merge/block_processing/test_process_execution_payload.py

tests/core/pyspec/eth2spec/test/merge/fork_choice/test_terminal_validity.py

tests/core/pyspec/eth2spec/test/merge/unit/test_unit.py

zilm13 · 2021-09-23T20:17:01Z

@djrtwo @hwwhww Updated everything according to your feedback, sorry if missed a thing, there was a lot of it.
Added test_on_merge_block fork choice tests for clients and turned on genesis and fork_choice tests for MERGE. Maybe there are another opportunities I'm missing to test clients? But it's all I see. I'm ready for merge )
Also we need consensus-spec-tests regen after this one is merged

hwwhww

@zilm13

I got errors once I added yield from to test_on_merge_block.py tests locally. Not sure how to fix the assert spec.get_head(store) == anchor_block.state_root correctly, but I'm happy to help fix the testgen, feel free to ping me if you have any questions!

tests/core/pyspec/eth2spec/test/merge/fork_choice/test_on_merge_block.py

tests/formats/fork_choice/README.md

tests/core/pyspec/eth2spec/test/merge/fork_choice/test_on_merge_block.py

zilm13 · 2021-09-24T18:09:28Z

@hwwhww thanks for the help, the tests were not actually run. Now fixed. Could you check it?

tests/core/pyspec/eth2spec/test/helpers/fork_choice.py

hwwhww · 2021-09-24T18:49:02Z

Once the above suggestion is applied, the new test case outputs look good to me! 🎉

zilm13 · 2021-09-24T22:42:03Z

@hwwhww the fix passed!

djrtwo · 2021-09-25T02:29:30Z

My apologies for not getting to this before the release today. I'll include them in the release monday and make sure consensus client teams know there are additional vectors available

hwwhww

Some minor refactoring suggestions. Otherwise, it looks good to me. 👍

hwwhww · 2021-09-27T04:30:27Z

tests/core/pyspec/eth2spec/test/merge/fork_choice/test_on_merge_block.py

+
+
+@with_phases([MERGE])
+@with_presets([MINIMAL], reason="mainnet `TERMINAL_TOTAL_DIFFICULTY` stub would cause overflow")


To be clear, only this test case would overflow with mainnet config stub. Maybe remove this decorator from other test cases?

tests/core/pyspec/eth2spec/test/merge/fork_choice/test_on_merge_block.py

hwwhww · 2021-09-27T06:02:22Z

configs/minimal.yaml

+# TBD, 2**256-2**10 is a placeholder
+TERMINAL_TOTAL_DIFFICULTY: 115792089237316195423570985008687907853269984665640564039457584007913129638912


second thought, I think it would be more useful & practical if we also set mainnet config to stub 2**256-2**10. (require spec change though)

/cc @djrtwo

tests/core/pyspec/eth2spec/test/merge/unittests/test_terminal_validity.py

tests/core/pyspec/eth2spec/test/merge/unittests/test_transition.py

djrtwo

I'm a bit confused about run_process_merge_execution_payload usage in test_terminal_validity

tests/core/pyspec/eth2spec/test/helpers/fork_choice.py

djrtwo · 2021-09-27T14:22:17Z

tests/core/pyspec/eth2spec/test/merge/unittests/test_terminal_validity.py

+    assert spec.is_valid_terminal_pow_block(pow_block, pow_parent)
+
+
+def run_process_merge_execution_payload(spec, block, parent_block, payload,


This doesn't run process_merge_execution_payload (which isn't a function I believe) nor process_execution_payload. Instead, it runs is_valid_terminal_pow_block.

I'm also unsure why we are yielding anything here. Is this a new vector type? And if it is, shouldn't a result -- True or False be provided?

It seems that this function is incorrectly named and that it should not be yielding any test vectors

djrtwo · 2021-09-27T17:22:32Z

closing in favor of #2630

#2598 + cleanups

zilm13 added 4 commits September 10, 2021 17:43

Merge branch 'dev' into tests/merge

f38750a

Fixed uint256 type misalign

14a3825

Extracted process_merge_execution_payload, terminal block validatio…

9acf60f

…n function

Added new Merge tests

7ff173b

mkalinin requested review from djrtwo, mkalinin, protolambda and ralexstokes and removed request for mkalinin September 14, 2021 08:54

zilm13 added 2 commits September 14, 2021 15:26

Fixed lint errors for merge tests

cf1bd6c

Renamed test_on_block -> test_terminal_validity to avoid misleading a…

b99720e

…s generator test

hwwhww added testing CI, actions, tests, testing infra bellatrix labels Sep 14, 2021

Enable genesis client tests for MERGE

829c6e7

zilm13 added 3 commits September 20, 2021 22:26

Merge branch 'hardcoded-ttd' into tests/merge

8bfa518

Changed terminal total difficulty to have some difficulty range for t…

9bd95c4

…ests without overflow

merge tests updated to be in line with transition_store removal

cdcf366

djrtwo reviewed Sep 22, 2021

View reviewed changes

hwwhww reviewed Sep 23, 2021

View reviewed changes

zilm13 added 5 commits September 23, 2021 22:22

Added on_merge_block client tests

b1aa227

Fixed lint errors for test_on_merge_block.py

239653e

Merge branch 'dev' into tests/merge

8ae078a

Polishing merge tests

1ecfc40

Fixed imports in merge tests

f3f1c86

hwwhww reviewed Sep 24, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/merge/fork_choice/test_on_merge_block.py Show resolved Hide resolved

update PowBlock generation with random hash

457b039

Fixed test_on_merge_block tests

f8b3a67

hwwhww reviewed Sep 24, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/helpers/fork_choice.py Outdated Show resolved Hide resolved

Make empty pow_block hash generation Python 3.8 compatible

deb4dbd

hwwhww approved these changes Sep 27, 2021

View reviewed changes

djrtwo reviewed Sep 27, 2021

View reviewed changes

djrtwo and others added 2 commits September 27, 2021 08:23

Apply suggestions from code review

c8d05c6

Apply the trivial suggestions

2fa595f

hwwhww mentioned this pull request Sep 27, 2021

#2598 + cleanups #2630

Merged

djrtwo closed this Sep 27, 2021

djrtwo added a commit that referenced this pull request Sep 27, 2021

Merge pull request #2630 from ethereum/tests/merge-clean-up

d34b79f

#2598 + cleanups

zilm13 deleted the tests/merge branch September 28, 2021 08:24

hwwhww mentioned this pull request Sep 30, 2021

[WIP] Mock Engine API req/resp in tests #2637

Closed



		@with_phases([MERGE])
		@with_presets([MINIMAL], reason="mainnet `TERMINAL_TOTAL_DIFFICULTY` stub would cause overflow")

		# TBD, 2256-210 is a placeholder
		TERMINAL_TOTAL_DIFFICULTY: 115792089237316195423570985008687907853269984665640564039457584007913129638912

		assert spec.is_valid_terminal_pow_block(pow_block, pow_parent)


		def run_process_merge_execution_payload(spec, block, parent_block, payload,

Conversation

zilm13 commented Sep 14, 2021

Uh oh!

zilm13 commented Sep 15, 2021

Uh oh!

djrtwo commented Sep 22, 2021

Uh oh!

djrtwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hwwhww left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zilm13 commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hwwhww left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zilm13 commented Sep 24, 2021

Uh oh!

Uh oh!

hwwhww commented Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zilm13 commented Sep 24, 2021

Uh oh!

djrtwo commented Sep 25, 2021

Uh oh!

hwwhww left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zilm13 commented Sep 23, 2021 •

edited

Loading

hwwhww left a comment •

edited

Loading

hwwhww commented Sep 24, 2021 •

edited

Loading