Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block class to vm #640

Merged
merged 2 commits into from
May 4, 2018
Merged

Block class to vm #640

merged 2 commits into from
May 4, 2018

Conversation

carver
Copy link
Contributor

@carver carver commented May 3, 2018

What was wrong?

Related to #599 & #507

  • state shouldn't care about block_class at all
  • variable mutation internal to method that applies transactions sequentially

How was it fixed?

  • moved block_class to VM
  • this concept of applying a series of data one on top of the other seems oft-repeated in blockchains, so made a reusable pipe_accumulator to handle this case, and hopefully others.

Cute Animal Picture

put a cute animal picture link inside the parentheses

@@ -0,0 +1,16 @@
def pipe_accumulator(base_state, transformer, inputs):
Copy link
Contributor Author

@carver carver May 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This felt like a good thing that might make sense to reuse a lot.

The idea is that we care about two things when applying a sequence of transforms: the final state (note I mean state in a general way, not VMState), and the list of artifacts produced along the way. In the context of applying transactions, we care about the final header, and all the receipts/computations produced along the way.

So with two inputs, like this:

              input0                    input1
                 |                        |
                 v                        v
state0 --> transform() -- --state1--> transform() -- --state2-->
                         |                          |            
                         v                          v           
                     artifact1                  artifact2 

We want state2 and (artifact1, artifact2). The base case of "no inputs" should generate an output of state0 and (, ).

I'll put that ^ diagram in the comments before merging


On the name: I also sort of like the name pipe_map for this method. Any other suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to drop this commit because I like the idea of the transform generating a state-diff better. I'll come back to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

  • accumulation_pipe
  • pipe_and_accumulate
  • transform_and_accumulate
  • apply_and_accumulate

@carver carver force-pushed the block-class-to-vm branch from 7a5f448 to bb5b969 Compare May 4, 2018 00:30
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/carver/py-evm/blob/bb5b969c0fccb4a8a91dfbd75440e129b37aaa4d/evm/vm/state.py#L265-L327

Are you planning to address this section in a subsequent PR or is this a WIP?

@carver
Copy link
Contributor Author

carver commented May 4, 2018

Since this PR got lighter without the pipe_accumulator stuff, I'll go ahead and add that now. Will ping when ready for review.

@carver carver force-pushed the block-class-to-vm branch from bb5b969 to 1bd7685 Compare May 4, 2018 18:40
@carver carver force-pushed the block-class-to-vm branch from 1bd7685 to 3f24a37 Compare May 4, 2018 18:52
@carver
Copy link
Contributor Author

carver commented May 4, 2018

Ok, ready for you @pipermerriam

@carver carver merged commit 9f7415a into ethereum:master May 4, 2018
@carver carver deleted the block-class-to-vm branch May 4, 2018 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants