Incorporate state_transition.py into state transition spec #1018

JustinDrake · 2019-05-01T11:08:40Z

This PR incorporates state_transition.py (along with a significant cleanup) into the state transition spec. This limits "magic" outside of the spec and addresses #946. We generally reduce fluff, increase readability, and reduce line count (20 lines in the spec, 140 lines overall). A bug is also fixed with the addition of assert state.slot < block.slot in state_transition.

The state transition spec should be reasonably self-contained, limiting the amount of "magic" outside of it. This PR is a first step in this direction, specifically for operation processing.

hwwhww

My first pass.

Generally, I think that's the right direction 👍, but I'd be inclined to favor to see more description in code comment to help understand. :)

specs/core/0_beacon-chain.md

hwwhww · 2019-05-02T03:38:17Z

specs/core/0_beacon-chain.md

+    process_randao(state, block)
+    process_eth1_data(state, block)
+    process_operations(state, block.body)
+    # verify_block_state_root(state, block)


I'd say enable it in the spec, and hack it in the tests.

/cc @djrtwo

a bit torn here.

options:

uncomment and leave as is

add verify_state_root boolean in process_block and state_transition and a note about in which context you would run it as True or False (receiving a block vs building a block)

have verify_block_state_root live outside of the state transition function entirely and as a note on block validity below.

I think I like (2) the best, but am open to debate.
*

My suggestion would be for the test bench to populate the state_root immediately prior to verification in case it is set to ZERO_HASH. That way we can keep the spec clean. Could that work?

some sort of monkey patch?

JustinDrake · 2019-05-02T10:18:17Z

Thanks @hwwhww for the good feedback :) I agree regarding comments. The state_transition function now has ~1 comment per line of code.

djrtwo

like it. a few notes

specs/core/0_beacon-chain.md

djrtwo · 2019-05-03T00:29:39Z

specs/core/0_beacon-chain.md

+    process_randao(state, block)
+    process_eth1_data(state, block)
+    process_operations(state, block.body)
+    # verify_block_state_root(state, block)


a bit torn here.

options:

uncomment and leave as is

add verify_state_root boolean in process_block and state_transition and a note about in which context you would run it as True or False (receiving a block vs building a block)

have verify_block_state_root live outside of the state transition function entirely and as a note on block validity below.

I think I like (2) the best, but am open to debate.
*

specs/core/0_beacon-chain.md

Co-Authored-By: JustinDrake <drakefjustin@gmail.com>

djrtwo · 2019-05-07T16:35:25Z

unsure of the exception handling. See #1059 (comment)

djrtwo · 2019-05-08T14:55:19Z

I think we should revert the exception handling here and get it merged and discuss otherwise in #1059

JustinDrake · 2019-05-08T18:16:26Z

I think we should revert the exception handling here

Done. Feel free to merge :)

djrtwo

removing approve to discuss state root validation

djrtwo · 2019-05-13T20:55:14Z

@JustinDrake We didn't decide on how to handle state root validation. I moved the process_state_root rfunction out to state_transition and added a flag. this seems natural to me for clients as they will certainly have a flag to be able to run either way, and defaulting to false translates easily to our current testing infrastructure.

Waiting for comment before merge

JustinDrake · 2019-05-14T05:16:33Z

I moved the process_state_root rfunction out to state_transition

Makes sense. I like the idea of inlining it!

and added a flag

We probably want to clean this up eventually but don't want to block this PR. Suggest merging :)

JustinDrake added 2 commits May 1, 2019 12:04

Start moving state_transition.py to state transitition spec

8b316c6

The state transition spec should be reasonably self-contained, limiting the amount of "magic" outside of it. This PR is a first step in this direction, specifically for operation processing.

Simplify state_transition.py

591a2b4

JustinDrake changed the title ~~Start moving state_transition.py to state transitition spec~~ Start moving state_transition.py to state transition spec May 1, 2019

Get rid of state_transition.py

5df79d7

JustinDrake changed the title ~~Start moving state_transition.py to state transition spec~~ Incorporate state_transition.py into state transition spec May 1, 2019

JustinDrake requested review from hwwhww and djrtwo and removed request for hwwhww May 1, 2019 12:21

JustinDrake added 3 commits May 1, 2019 13:29

clean up

2e63a9b

Cleanup

9b24d06

Fix

7980cf3

JustinDrake added general:presentation Presentation (as opposed to content) general:bug Something isn't working labels May 1, 2019

hwwhww reviewed May 2, 2019

View reviewed changes

Update 0_beacon-chain.md

9f2cdd9

djrtwo reviewed May 3, 2019

View reviewed changes

Add HW and Danny comments

197a720

JustinDrake mentioned this pull request May 3, 2019

Epoch processing is too eager #1041

Closed

Update 0_beacon-chain.md

4c1073f

JustinDrake mentioned this pull request May 4, 2019

Improve epoch processing within state transition #1043

Closed

hwwhww mentioned this pull request May 7, 2019

Avoid side effect in state transition with mutable object #1059

Closed

hwwhww reviewed May 7, 2019

View reviewed changes

specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved

specs/core/0_beacon-chain.md Show resolved Hide resolved

hwwhww and others added 3 commits May 7, 2019 09:01

Update specs/core/0_beacon-chain.md

f371dae

Co-Authored-By: JustinDrake <drakefjustin@gmail.com>

Implement HW's exception-handling suggestion

50009ea

Merge branch 'dev' into JustinDrake-patch-14

9eeca0c

Revert exception handling

c37157e

djrtwo added 4 commits May 8, 2019 14:49

remove unnecessary import of copy

8da4b81

Merge branch 'dev' into JustinDrake-patch-14

27c8e41

add process_slots usage to validator guide

7bb85a6

lint

a0a2aa9

djrtwo approved these changes May 13, 2019

View reviewed changes

add flag for validate state root

5ba90d6

djrtwo requested changes May 13, 2019

View reviewed changes

Update 0_beacon-chain.md

c60635d

djrtwo approved these changes May 14, 2019

View reviewed changes

djrtwo merged commit 23b1cb2 into dev May 14, 2019

djrtwo deleted the JustinDrake-patch-14 branch May 14, 2019 19:36

djrtwo mentioned this pull request May 15, 2019

Make phase 1 spec executable #1061

Merged

mratsim mentioned this pull request Jun 27, 2019

v0.7.x state transition refactoring status-im/nimbus-eth2#299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorporate state_transition.py into state transition spec #1018

Incorporate state_transition.py into state transition spec #1018

JustinDrake commented May 1, 2019 •

edited

Loading

hwwhww left a comment

hwwhww May 2, 2019

djrtwo May 3, 2019

JustinDrake May 3, 2019 •

edited

Loading

djrtwo May 5, 2019

JustinDrake May 5, 2019

JustinDrake commented May 2, 2019 •

edited

Loading

djrtwo left a comment

djrtwo May 3, 2019

djrtwo commented May 7, 2019

djrtwo commented May 8, 2019

JustinDrake commented May 8, 2019

djrtwo left a comment

djrtwo commented May 13, 2019

JustinDrake commented May 14, 2019

Incorporate state_transition.py into state transition spec #1018

Incorporate state_transition.py into state transition spec #1018

Conversation

JustinDrake commented May 1, 2019 • edited Loading

hwwhww left a comment

Choose a reason for hiding this comment

hwwhww May 2, 2019

Choose a reason for hiding this comment

djrtwo May 3, 2019

Choose a reason for hiding this comment

JustinDrake May 3, 2019 • edited Loading

Choose a reason for hiding this comment

djrtwo May 5, 2019

Choose a reason for hiding this comment

JustinDrake May 5, 2019

Choose a reason for hiding this comment

JustinDrake commented May 2, 2019 • edited Loading

djrtwo left a comment

Choose a reason for hiding this comment

djrtwo May 3, 2019

Choose a reason for hiding this comment

djrtwo commented May 7, 2019

djrtwo commented May 8, 2019

JustinDrake commented May 8, 2019

djrtwo left a comment

Choose a reason for hiding this comment

djrtwo commented May 13, 2019

JustinDrake commented May 14, 2019

JustinDrake commented May 1, 2019 •

edited

Loading

JustinDrake May 3, 2019 •

edited

Loading

JustinDrake commented May 2, 2019 •

edited

Loading