Merge test vectors: enable phase0 tests for Merge + start on new testing #2380

protolambda · 2021-05-04T22:52:30Z

Changes:

Add merge to forks list with enabled testing and test-vector outputs
Update test generators to include merge:
- SSZ static tests now respect TESTGEN_FORKS again
- epoch processing (inherits phase0 tests)
- finality (inherits phase0 tests)
- operations (inherits phase0 tests, added execution payload testing)
- rewards (inherits phase0 tests)
- sanity (inherits phase0 tests, added block tests)
Helper method to create empty execution payload on top of the latest state
Update beacon-block construction to add in a valid empty execution payload when the merge spec is detected.
- TODO: may need to be a zeroed execution payload for the time during the merge transition. For next PR.
Refactor merge spec (no breaking consensus changes):
- Isolate the execution payload input for execution payload processing. There is no good reason to include the full block body. The old version makes forking the Merge code with code-reuse much more difficult.
- The if statement for processing simply moves to outside the processing function. So the processing is conditional, instead of a possible no-op.
Implement execution-payload tests
- Scaffold edge cases to test
  - Note: left 1 minimal test case. Further testing in next PR.
- Define test format
Implement block tests
- Just a minimal scaffold, needs more work in later PR

TLDR: Port phase0 testing for Merge, enable Merge test generators.

hwwhww · 2021-05-05T12:29:43Z

specs/merge/beacon-chain.md

+    if is_transition_completed(state) or is_transition_block(state, block):
+        process_execution_payload(state, block.body.execution_payload)  # [New in Merge]


Two issues:

Looking at tests/formats/operations/README.md and I just realized that we put sync_aggregate as part of "operation" tests but process_sync_committee is not part of the process_operations function like execution_payload.

IMO it's good to consider sync_aggregate as an operation.

If we want to simplify it, I think execution_payload could also be considered as an operation since it's essentially "transactions".

I can see why it looks good to extract the is_transition_completed(state) or is_transition_block(state, block) condition checks from process_execution_payload, but it makes it a bit unclear where/how to test these conditions in test cases.

What do you think about:

Move process_sync_committee and process_execution_payload calls into process_operations.

Revert the if is_transition_completed(state) or is_transition_block(state, block) checks extraction.

Regarding process_sync_committee: sure, but let's keep that separate from merge-testing for now. Moving the process_execution_payload to process_operations would be good too. I'm honestly surprised we didn't do the same for the Randao and Eth1Data functionality.

The if-statement was purposefully extracted from the process_execution_payload function: this way the function input can just be an ExecutionPayload, and not a complete beacon block body. This helps with future compatibility and reduces test complexity (We don't have to set up a complete beacon block for every single test case). We can test that one if-statement as part of the regular full beacon block testing. And in future phases after the merge, we may even remove the if-statement entirely, since the merge-transition check will always be true.

For (2), it might make sense to put it behind a function is_execution_enabled(state, block). This is semantically cleaner for any chain/setting that doesn't have the transition. They can just override the function to return True and as you mentioned, in the future we can just remove it.

djrtwo

Nice!

I think there's a bug in build_empty_block_for_next_slot and I'm not certain if we are actually hitting process_execution_payload. Didn't do any testing. Check out my comments. Hit me up. happy to discuss in DM too.

djrtwo · 2021-05-05T22:17:19Z

specs/merge/beacon-chain.md

+    if is_transition_completed(state) or is_transition_block(state, block):
+        process_execution_payload(state, block.body.execution_payload)  # [New in Merge]


For (2), it might make sense to put it behind a function is_execution_enabled(state, block). This is semantically cleaner for any chain/setting that doesn't have the transition. They can just override the function to return True and as you mentioned, in the future we can just remove it.

tests/core/pyspec/eth2spec/test/context.py

djrtwo · 2021-05-05T22:54:13Z

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

+        block_hash=spec.Hash32(),
+        parent_hash=latest.block_hash,
+        coinbase=spec.Bytes20(),
+        state_root=latest.state_root,  # no changes to the state


Is this certain to be the case?

Does the consensus not write anything to state automatically? The recent block_roots being included in state is something that comes to mind, like EIP 210. I know 210 isn't deployed but I'm unsure if there isn't anything else like that in place

There may be something yes, although I'm not entirely sure either. The account tree definitely does not change (no block rewards on eth1 state anymore), and the storage/values/code/nonce of accounts only changes with transactions. Can't find anything in the yellow paper that would otherwise touch the eth1 world state.

I'll open an issue to investigate this more. Maybe this new possibility of a duplicate state-root in eth1 blocks is untested/dangerous?

might be. at least it might not be expected

I think we've touched on this a tiny bit. Peter mentioned that it isn't the case today because of issuance/coinbase but that it might change in this context. I think they can handle it but it's something to highlight as a design consideration/edge-case for execution engines

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

djrtwo · 2021-05-05T22:57:08Z

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

+        transactions=empty_txs,
+    )
+    # TODO: real RLP + block hash logic would be nice, requires RLP and keccak256 dependency however.
+    payload.block_hash = spec.Hash32(spec.hash(payload.hash_tree_root() + b"FAKE RLP HASH"))


The htr would not be used in the correct functionality, right?

Right, it's just a short-cut to make a hash that is unique to the payload. Encoding and flat-hashing would work to. I think we may just want to do the real RLP thing though. The current thing works for now, but it's nice if it's correct, especially if we get eth1-eth2 combination clients that run these tests.

We might want to decouple this shortcut and use it as a helper.

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

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

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

djrtwo · 2021-05-05T23:29:18Z

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

+@spec_state_test
+def test_success_first_payload(spec, state):
+    next_slot(spec, state)
+    assert not spec.is_transition_completed(state)


Maybe assert that it is a transition block too

There is no block processing here, just the execution payload, so as-is it cannot be asserted.

Maybe we should start using the new run_block_processing_to helper to simulate the block behavior up to the execution-payload processing, I can discuss this with Mikhail for the next PR, but I prefer to keep this PR simple.

Is this test case supposed to test the transition?

process_execution_payload has been refactored to not care about the transition logic. So yes, it tests the empty header to a real header but it doesn't test that the transition go/no-go logic works

A nit: then we likely should not use transition spec helpers to check the pre-state.

ah, but we want to test a range of tests/pre-states/inputs directly on this function. One of which when state has an empty header which maps to not spec.is_transition_completed. Even though process_execution_payload does not check this, we do want to ensure we have a test cast that is in this pre-condition.

If we don't use that helper, we'd want to check state.latest_execution_header == ExecutionHeader(). Or directly just set it.

Ok, I see. This is more likely to be done in a separate PR anyway. Though, I would rather set test case preliminaries in the test case itself than expect them from the outside.

Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>

djrtwo

great start! happy to get this merged pretty soon.

specs/merge/beacon-chain.md

mkalinin

Great work so far! Let's merge and keep work on execution payload tests in #2385

protolambda added 5 commits May 5, 2021 00:15

include merge in generators

f37f9a3

update spec test constants for merge

ab693c9

update execution-payload processing to isolate payload from block body

521cffc

start testing of merge functionality

25d0d67

update doc about format

36032fd

protolambda added scope:CI/tests/pyspec Bellatrix CL+EL Merge labels May 4, 2021

Adjust is_transition_block call in fork-choice

79fc411

hwwhww reviewed May 5, 2021

View reviewed changes

protolambda added 7 commits May 5, 2021 15:35

introduce merge fork version

e2be761

update test runner to handle merge phase

470c6dc

fix transactions field in exec payload helper

ff3a82e

update altair tests to not collide with Merge + fix merge test triggers

865d7db

fix merge forkchoice tests with mock get_pow_block

cc11328

fix forkchoice unittest not recognizing merge spec

00cd1c3

Minimal execution payload test, more merge-specific testing in later PR

2ef6291

protolambda changed the title ~~Merge test vectors (work in progress)~~ Merge test vectors: enable phase0 tests for Merge + start on new testing May 5, 2021

protolambda mentioned this pull request May 5, 2021

Execution payload tests #2385

Merged

Lint fixes for merge testing update

56bcb63

protolambda marked this pull request as ready for review May 5, 2021 15:08

protolambda requested a review from ralexstokes May 5, 2021 19:35

fix old ssz-static todo comment

8ac59b7

djrtwo reviewed May 5, 2021

View reviewed changes

protolambda and others added 2 commits May 6, 2021 02:23

is_execution_enabled function + misc review fixes

76b5974

Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>

remove merge-test exec-payload trigger debug helper

42733b7

protolambda mentioned this pull request May 6, 2021

Merge side-effect: duplicate execution state-root concern #2389

Closed

1 task

djrtwo approved these changes May 6, 2021

View reviewed changes

mkalinin reviewed May 7, 2021

View reviewed changes

specs/merge/beacon-chain.md Show resolved Hide resolved

mkalinin approved these changes May 7, 2021

View reviewed changes

djrtwo merged commit 2539d4e into dev May 10, 2021

djrtwo deleted the merge-test-vectors branch May 10, 2021 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge test vectors: enable phase0 tests for Merge + start on new testing #2380

Merge test vectors: enable phase0 tests for Merge + start on new testing #2380

protolambda commented May 4, 2021 •

edited

Loading

hwwhww May 5, 2021 •

edited

Loading

protolambda May 5, 2021

djrtwo May 5, 2021

djrtwo left a comment

djrtwo May 5, 2021

djrtwo May 5, 2021

protolambda May 6, 2021

djrtwo May 6, 2021

djrtwo May 5, 2021

protolambda May 5, 2021

mkalinin May 6, 2021

djrtwo May 5, 2021

protolambda May 6, 2021

mkalinin May 6, 2021

djrtwo May 6, 2021

mkalinin May 6, 2021

djrtwo May 7, 2021

mkalinin May 7, 2021

djrtwo left a comment

mkalinin left a comment

		if is_transition_completed(state) or is_transition_block(state, block):
		process_execution_payload(state, block.body.execution_payload) # [New in Merge]

Merge test vectors: enable phase0 tests for Merge + start on new testing #2380

Merge test vectors: enable phase0 tests for Merge + start on new testing #2380

Conversation

protolambda commented May 4, 2021 • edited Loading

hwwhww May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo left a comment

Choose a reason for hiding this comment

mkalinin left a comment

Choose a reason for hiding this comment

protolambda commented May 4, 2021 •

edited

Loading

hwwhww May 5, 2021 •

edited

Loading