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

Implement State object #236

Closed
pipermerriam opened this issue Dec 20, 2017 · 6 comments
Closed

Implement State object #236

pipermerriam opened this issue Dec 20, 2017 · 6 comments

Comments

@pipermerriam
Copy link
Member

What is wrong?

Currently, the VM class is part of the constructor for the Computation object. Here are the places where it is currently used.

evm/logic/block.py:7:    block_hash = computation.vm.get_ancestor_hash(block_number)
evm/logic/block.py:13:    computation.stack.push(computation.vm.block.header.coinbase)
evm/logic/block.py:17:    computation.stack.push(computation.vm.block.header.timestamp)
evm/logic/block.py:21:    computation.stack.push(computation.vm.block.header.block_number)
evm/logic/block.py:25:    computation.stack.push(computation.vm.block.header.difficulty)
evm/logic/block.py:29:    computation.stack.push(computation.vm.block.header.gas_limit)
evm/logic/system.py:87:    computation.vm.logger.debug(
evm/logic/system.py:141:            computation.vm.logger.debug(
evm/vm/forks/spurious_dragon/utils.py:22:        yield computation.vm.block.header.coinbase
evm/vm/computation.py:76:        self.vm = vm
evm/vm/computation.py:197:            child_computation = self.vm.apply_create_message(child_msg)
evm/vm/computation.py:199:            child_computation = self.vm.apply_message(child_msg)
evm/vm/computation.py:280:        with self.vm.state_db(read_only, self.msg.access_list) as state_db:

Giving computation direct access to the VM is problematic for a number of reasons.

  • poor isolation for what data and APIs are available during EVM execution.
  • makes implementation of stateless clients difficult.
  • makes VM execution inherently non-pure.

How can it be fixed

To fix this, lets create a new State object. Similar to how the Transaction class is specified for each VM, the State object should be similarly overrideable.

  • VM.get_state_class()
  • VM.get_state()
  • VM.state (this could be a convenience property that just delegates to VM.get_state().

The State object will need the following things.

  • Access to the current block header information (coinbase/timestamp/block_number/difficulty/gas_limit.
  • The ancestry hashes for the current block number.
  • Access to the StateDB

The State object will also need the following APIs available to it.

  • VM.apply_message
  • VM.apply_create_message

I think the best way to accomplish this is to relocate the following APIs to live directly on the State object itself, removing them from the VM object.

  • VM.apply_message
  • VM.apply_create_message
  • VM.apply_computation

Note that VM.apply_computation needs access to VM.get_opcode_fn() and VM.precompiles. This indicates we'll need to supply the State object with a way to get at this data. The simplest solution that comes to mind is to add them as extra arguments:

class State:
    def apply_computation(self, message, opcodes, precompiles):
        ...
@pipermerriam
Copy link
Member Author

@hwwhww I was thinking you (or someone on the sharding team) would be a good fit for implementing this since you'll have a better idea of what's needs to be built on top of it. Otherwise, I'm happy to take this on.

@hwwhww
Copy link
Contributor

hwwhww commented Dec 21, 2017

@pipermerriam
It's really helpful and clear, thank you!
I will start this task first (in master).

@hwwhww
Copy link
Contributor

hwwhww commented Dec 21, 2017

Worklog

In /tests/json-fixtures/test_virtual_machine.py, it is configured to not actually apply messages.

apply_message=apply_create_message_for_testing,
apply_create_message=apply_create_message_for_testing,

But these two function were moved from VM in to State.

  1. Because we use VM.get_state() to get state instance, simply monkey patching on HomesteadVMForTesting doesn't works.
  2. Set configuration for State object via VM may work, but looks hassle if it's only for tests....?

@pipermerriam
Copy link
Member Author

With the idea that the State object is configurable for each VM, I think it's reasonable to do the following for those tests.

  • Create a specific State object subclass for the given VM that has these methods overridden with the testing versions.
  • Create a specific VM object subclass for the geven VM that has the augmented State object configured as it's default state object.

Thoughts? Not sure if I missed something. I'm open to alternate solutions.

@hwwhww
Copy link
Contributor

hwwhww commented Dec 22, 2017

@pipermerriam Your solution works well and clean! I apply it by replacing _state_class:

HomesteadStateForTesting = HomesteadState.configure(
    name='HomesteadStateForTesting',
    apply_message=apply_message_for_testing,
    apply_create_message=apply_create_message_for_testing,
    get_ancestor_hash=get_block_hash_for_testing,
)
HomesteadVMForTesting = HomesteadVM.configure(
    name='HomesteadVMForTesting',
    _state_class=HomesteadStateForTesting,
)

Thank you so much.

@hwwhww
Copy link
Contributor

hwwhww commented Jan 13, 2018

close via #247

@hwwhww hwwhww closed this as completed Jan 13, 2018
@hwwhww hwwhww moved this from In progress to Done in Sharding Phase 1 - Stage 1 Jan 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

No branches or pull requests

2 participants