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

Piper/refactor api for access to state db #621

Conversation

pipermerriam
Copy link
Member

replaces #611

What was wrong?

The original context manager API for dealing with the account_db was built to make sure that the state_root stayed updated.

I'm of the opinion that 1) we no longer need this mechanism and 2) that the readability overhead isn't worth it anymore.

How was it fixed?

Still a WIP but the plan is to take steps towards making State fully immutable. Things like apply_transaction and/or execute_transaction will return either state_root, computation or maybe state_diff, computation.

All of the other things like creation of the receipt will move upwards in the stack.

Cute Animal Picture

put a cute animal picture link inside the parentheses

@pipermerriam pipermerriam force-pushed the piper/refactor-api-for-access-to-state-db branch from 40d9a7a to 6462b37 Compare April 30, 2018 23:20
@pipermerriam pipermerriam force-pushed the piper/refactor-api-for-access-to-state-db branch from 825803e to 3540dd0 Compare May 1, 2018 00:18
@pipermerriam
Copy link
Member Author

pipermerriam commented May 1, 2018

@carver mind looking this over. The gas estimation tests are now returning different numbers (looks to be consistently off by 12 gas). First pass on my part didn't surface anything obvious. Hoping fresh eyes find something I didn't.

@pipermerriam pipermerriam requested a review from carver May 1, 2018 16:38
@pipermerriam
Copy link
Member Author

Problems that remain or surfaced with this PR.

  • It's still easy to forget to call account_db.persist().
    • Making the account_db ephemeral within the State object and using a context manager API for initializing the root checkpoint and persisting it should fix this.
    • Not exposing the AccountDB.state_root until AccountDB.persist() has been called might also work.
  • The State object has block stuff in it.
    • This should all be moved up to the VM
  • The State.gas_used can probably be removed.
  • The State.state_root is awkward and should probably be removed (see above about making account_db ephemeral
  • The VM.block property is problematic statefulness.
  • The VM.pack_block method is awkward.

return min(gas_plus_buffer, state.gas_limit)


double_execution_cost = execute_plus_buffer(2)
Copy link
Member Author

@pipermerriam pipermerriam May 1, 2018

Choose a reason for hiding this comment

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

This wasn't used anywhere and the new gas estimation API using the binary search seemed categorically superior so... removing this.

Copy link
Contributor

@carver carver left a comment

Choose a reason for hiding this comment

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

Cool, good progress. Just one piece that I think is redundant.

computation = state.do_call(transaction)
snapshot = state.snapshot()
try:
computation = state.do_call(transaction)
Copy link
Contributor

Choose a reason for hiding this comment

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

do_call() already does the snapshot/revert internally. Based on the name (and its presumed equivalence to rpc api's eth_call), it seems reasonable to expect do_call() to continue handling that. So I think this method's change can be undone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, we should be able to remove do_call if we get enough of the stateless/pure stuff done right. Just calling down into one of the deeper APIs which doesn't mutate and throwing away the state diff (or something like that).

@pipermerriam pipermerriam merged commit fa5817b into ethereum:master May 1, 2018
@pipermerriam pipermerriam deleted the piper/refactor-api-for-access-to-state-db branch May 1, 2018 19:28
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