Accept genesis state and parameter overrides #123

KPrasch · 2018-08-19T08:01:25Z

What was wrong?

The genesis parameters, including the block gas limit is hardcoded, indicated by Issue #88.

How was it fixed?

Allow an optional dictionary of genesis parameter overrides to be passed into the backend's __init__.
Only existing default genesis parameters can be overridden, or a ValueError will be raised.

Cute Animal Picture

pipermerriam · 2018-08-20T17:11:02Z

@KPrasch can you rebase this now that #122 has been merged.

KPrasch · 2018-08-20T17:59:36Z

Awesome, and done! Thanks @pipermerriam

pipermerriam · 2018-08-20T19:07:49Z

eth_tester/backends/mock/factory.py

@@ -259,6 +259,15 @@ def make_genesis_block():
        "uncles": [],
    }

+    if overrides is not None:
+        if not (all(bool(override in genesis_block) for override in overrides)):


It takes a litlte extra work but I think we can improve the error message below and the readability of this code.

allowed_fields = set(genesis_block.keys()) override_fields = set(overrides.keys()) unexpected_fields = tuple(sorted(override_fields.difference(allowed_fields))) if unexpected_fields: raise ValueError( "The following invalid fields were supplied to override the genesis parameters: {0}.".format(unexpected_fields) )

pipermerriam · 2018-08-20T19:08:46Z

eth_tester/backends/mock/factory.py

-def make_genesis_block():
-    return {
+def make_genesis_block(overrides=None):
+    genesis_block = {


I'd suggest using the name default_genesis_block. This will work well in conjunction with removing the mutation via update later in this function.

pipermerriam · 2018-08-20T19:09:31Z

eth_tester/backends/mock/factory.py

+            fields = ', '.join(genesis_block)
+            error = "Invalid genesis overrides; Valid parameters are {}".format(fields)
+            raise ValueError(error)
+        genesis_block.update(overrides)


Very minor but we prefer non-mutative approaches. You can use cytoolz.merge as follows.

from cytoolz import merge genesis_block = merge(default_genesis_block, overrides)

pipermerriam · 2018-08-20T19:11:48Z

eth_tester/main.py

@@ -505,8 +505,9 @@ def revert_to_snapshot(self, snapshot_id):
        for log_filter in self._log_filters.values():
            self._revert_log_filter(log_filter)

-    def reset_to_genesis(self):
-        self.backend.reset_to_genesis()
+    def reset_to_genesis(self, genesis_parameter_overrides=None, genesis_state_overrides=None):


I'm thinking that we should not expose this API at the ETHTester level and only support it at the backend level. It seems reasonable that some backends might not support custom genesis params and I think it is reasonable to require manual instantiation of the backend when you wish to override genesis parameters.

pipermerriam · 2018-08-20T19:33:20Z

eth_tester/backends/pyevm/main.py

@@ -278,7 +288,7 @@ class PyEVMBackend(object):
    chain = None
    fork_config = None

-    def __init__(self):
+    def __init__(self, genesis_parameter_overrides=None):


I think it would be better to just require providing the fully merged genesis parameters. I believe that will simplify a lot of these apis, removing the concept of merging the overrides into the base parameters. We should however provide an API for generating the merged parameters to make this easy for users.

Maybe a classmethod or staticmethod on the main PyEVMBackend class?

class PyEVMBackend: @staticmethod def generate_genesis_params(overrides=None): ... @staticmethod def generate_genesis_state(overrides=None): ...

Then the user workflow would be something like:

from eth_tester import EthereumTester, PyEVMBackend state = PyEVMBackend.generate_genesis_state(state_overrides) genesis_params = PyEVMBackend.generate_genesis_params(genesis_overrides) tester = EthereumTester(backend=PyEVMBackend(genesis_params, state)

it's a bit verbose, but not terrible and it should simplify the other APIs.

👍 - I 100% agree.

KPrasch · 2018-08-20T23:25:57Z

Thanks for the review - excellent feedback / ideas @pipermerriam - Ill make these changes

KPrasch · 2018-08-29T09:54:59Z

OK - Ready for another review 🤠

I kept the changeset mostly focused on genesis parameters. I included both staticmethods for generating custom genesis params, and a classmethod: PyEVM.from_genesis_overrides as an alternate constructor. Parameter merging is non-mutative, and the overriding API is used at the backend level, for simplicity.

voith · 2018-08-29T13:01:39Z

eth_tester/main.py

@@ -605,7 +605,7 @@ def create_log_filter(self, from_block=None, to_block=None, address=None, topics

        if is_integer(raw_from_block):
            if is_integer(raw_to_block):
-                upper_bound = raw_to_block
+                upper_bound = raw_to_block + 1


I thought I had fixed this in #121.
How did this re-appear?

does this PR need a rebase?

Hello!

I reset this module to the wrong ref when this PR was a WIP. I rebased a fix.
Good catch, and Thanks for the review. 🤠

…enesis methods across the codebase

…esis merge

KPrasch · 2018-09-05T20:00:40Z

Checking In - Does this PR seem like a reasonable approach, or do I need make additional modifications?

voith · 2018-09-16T15:16:32Z

@carver @pipermerriam Can you please review this? This is a very useful feature

carver

Cool, looks like it's nearly there!

carver · 2018-09-17T18:36:02Z

eth_tester/backends/pyevm/main.py

@@ -119,14 +120,14 @@ def get_default_account_keys():


 @to_dict
-def generate_genesis_state(account_keys):
+def generate_genesis_state(account_keys, overrides=None):  # TODO: Merge genesis state overrides


If you don't intend to do the genesis state changes in this PR, can you:

remove the TODO

open an issue (suggesting this approach)

remove this overrides keyword arg, which implies that the feature works

carver · 2018-09-17T18:39:46Z

eth_tester/backends/pyevm/main.py

+        return get_default_genesis_params(overrides=overrides)
+
+    @staticmethod
+    def generate_genesis_state(overrides=None):


Another cleanup: delete overrides

carver · 2018-09-17T18:39:56Z

eth_tester/backends/pyevm/main.py

+    @staticmethod
+    def generate_genesis_state(overrides=None):
+        account_keys = get_default_account_keys()
+        return generate_genesis_state(account_keys=account_keys, overrides=overrides)


delete overrides

carver · 2018-09-17T18:40:04Z

eth_tester/backends/pyevm/main.py

+        return generate_genesis_state(account_keys=account_keys, overrides=overrides)
+
+    @classmethod
+    def from_genesis_overrides(cls, parameter_overrides=None, state_overrides=None):


delete state_overrides

carver · 2018-09-17T18:40:14Z

eth_tester/backends/pyevm/main.py

+    @classmethod
+    def from_genesis_overrides(cls, parameter_overrides=None, state_overrides=None):
+        params = cls.generate_genesis_params(overrides=parameter_overrides)
+        state = cls.generate_genesis_state(overrides=state_overrides)


delete state_overrides

carver · 2018-09-17T18:43:43Z

tests/backends/test_pyevm.py

-    pass
+
+    def test_override_genesis_parameters(self, eth_tester):
+        super().test_reset_to_genesis(eth_tester)


It strikes me as odd to run a test directly, especially inside another test. Maybe make a helper function that does a genesis reset, which both tests call.

carver · 2018-09-17T18:46:00Z

tests/backends/test_pyevm.py

+
+        # Or use the alternate constructor
+        pyevm_backend = PyEVMBackend.from_genesis_overrides(parameter_overrides=param_overrides)
+        assert pyevm_backend.chain.header._gas_limit == 4745362


These two lines should probably be pulled out into a separate test, so that pyevm_backend from line 43 gets tested in the EthereumTester integration below.

carver · 2018-09-17T18:51:24Z

tests/backends/test_pyevm.py

+
+        # Initialize pyevm backend with genesis params
+        pyevm_backend = PyEVMBackend(genesis_params)
+        assert pyevm_backend.chain.header._gas_limit == 4745362


How about using a public API instead, in case this private variable gets refactored? Something like:

genesis_block = pyevm_backend.get_block_by_number(0) assert genesis_block.header.gas_limit == 4750000

letting Jason take this.

…account states at PyEVM init-time

…Cleanup imports

KPrasch · 2018-09-18T19:57:12Z

@carver - Thanks for the review - I'd like to finish the state overrides also, which I just pushed.

…overrides.

carver

Sweet. Any thoughts on how important it is to be able to have different account state in two different accounts in the same genesis?

Like account A has balance 1, and B has balance 2. If that's likely to be important, let's figure out a good API for it now.

cc @voith

carver · 2018-09-18T20:23:19Z

eth_tester/backends/pyevm/main.py

+        genesis_params = get_default_genesis_params()
+
+    if genesis_state:
+        accounts = len(genesis_state)


If you like, num_accounts could be a little clearer as a name.

carver · 2018-09-18T20:37:12Z

eth_tester/backends/pyevm/main.py

+        return get_default_genesis_params(overrides=overrides)
+
+    @staticmethod
+    def generate_genesis_state(overrides=None, accounts=None):


It seems likely to cause confusion to have two methods of the same name (generate_genesis_state), especially in the same file. Maybe the other method could be called generate_genesis_state_for_keys.

Also, num_accounts here would be clearer as a variable name.

carver · 2018-09-18T20:51:50Z

eth_tester/backends/pyevm/main.py

+        return generate_genesis_state(account_keys=account_keys, overrides=overrides)
+
+    @classmethod
+    def from_genesis_overrides(cls, parameter_overrides=None, state_overrides=None, accounts=None):


I count three new APIs that are different ways of accomplishing similar things, maybe four if you count the generate_genesis_state_for_keys method. That means a lot of new documentation and examples and helping people figure out which one they should use in which situation. Any way we can tighten this up a bit? Ideally there would be one preferred way of setting genesis info.

For example, maybe this method shouldn't exist, and if someone wants to override both parameters and state, then the preferred way would be explicitly, like:

params = PyEVMBackend.generate_genesis_params(overrides=parameter_overrides) state = PyEVMBackend.generate_genesis_state(overrides=state_overrides, accounts=accounts) backend = PyEVMBackend(genesis_parameters=params, genesis_state=state)

That would be one less method to document, and one less question for where a new user should start when they want to override genesis info. Sure, it's three lines instead of one, but they decompose nicely, into understanding the other public APIs.

Personally, I like the simplicity of a one-liner in this situation - It even reduces confusion in some cases, IMO. I also understand that this does add an opportunity for confusion, and additional maintenance.

KPrasch · 2018-09-18T21:28:07Z

Having multiple states specified may indeed be useful. Perhaps if the state_overrides dictionary has more than one element, we can create an equal number of accounts, mapping the overrides to each new account's state (although order will not be preserved in < python 3.6).

carver · 2018-09-18T23:49:13Z

Having multiple states specified may indeed be useful.

Cool, how about something like this API...

This creates three account duplicate states, with a balance of 10**20:

state = PyEVMBackend.generate_genesis_state_duplicates(overrides=dict(balance=10**20), num_accounts=3)

This creates 4 accounts, the first with balance 10**20, the second with nonce 2, and two more default accounts:

state = PyEVMBackend.generate_genesis_state(override_accounts=(
  dict(balance=10**20),
  dict(nonce=2),
  {},
  {},
))

voith · 2018-09-19T21:32:14Z

tests/backends/test_pyevm.py


 from eth_tester import (
    EthereumTester,
    PyEVMBackend,
 )
+from eth_tester.backends.pyevm.main import generate_genesis_state_for_keys, get_default_account_keys, get_default_genesis_params


can you wrap this line?
too bad that eth-tester doesn't have isort!

voith · 2018-09-19T21:42:19Z

Any thoughts on how important it is to be able to have different account state in two different accounts in the same genesis?

I can't think of an immediate use case where I'd need different account states. However, having functionality that allows replicating an actual genesis block would be awesome!

Although what I would really love to see happen is that there should be an optional way to just provide an account address instead of the backend generating one for you. This helps when you're trying to sign a transaction and you have the private key of any genesis account. Currently, I do this in a very hacky way i.e I keep a mapping of privatekeys and default genessis accounts.

voith · 2018-09-19T21:44:08Z

overall great work @KPrasch! I am really looking forward to use this feature. Currently I have to monkeypatch some eth-tester code, specifically the gasLimit .

carver · 2018-09-19T22:00:13Z

However, having functionality that allows replicating an actual genesis block would be awesome!

You mean pass in a json-encoded string (the genesis file) to some classmethod, and you get a PyEVMBackend instance with that genesis block? I like it.

Maybe that should even be our primary public API, and these PR's methods are the tools we use to implement it. It seems like that API requires the least custom knowledge about eth-tester, since the genesis file format is a (de facto) global standard.

voith · 2018-09-19T22:07:25Z

You mean pass in a json-encoded string (the genesis file) to some classmethod, and you get a PyEVMBackend instance with that genesis block?

Yeah, I got this idea from pyethereum: https://github.com/ethereum/pyethereum/blob/develop/genesis_frontier.json

This will solve my account handling problem too.

carver · 2018-09-19T23:58:09Z

Cool, so I'm imagining an API like

backend = PyEVMBackend.from_genesis({'gas_limit': ... })

# also accepts the equivalent json-encoded string:
backend = PyEVMBackend.from_genesis('{"gas_limit": ... }')

The biggest problem I see with the approach is the way it handles hosted keys. Basically, if you specify a genesis file, then there's no way to also have PyEVMBackend host your keys for you. You would always have to pre-sign the transactions.

Some options:

Suck it up: if you want to specify a genesis file, then you need to manage your own private keys (which isn't too bad thanks to web3.py, but still more setup work than the alternative)
Add extra hosted accounts: this makes the API a little messier, and kind of pollutes the idea that you get exactly the genesis file you specified, but it is an easy way to add hosted keys. Also, it doesn't have include a clean API for custom state on your hosted keys. For that, refer back to "Suck it up". :)

Example of option 2:

backend = PyEVMBackend.from_genesis({'gas_limit': ... }, num_hosted_accounts=10)

num_hosted_accounts would default to 0. So if you don't specify hosted accounts, you don't get any. Then you would get the exact same starting state root as any other node that imports a genesis file. If num_hosted_accounts > 0, then from_genesis() would add additional accounts to the starting state with plenty of balance, and keep the private keys for you. (as it does now)

I'm on the fence about these two. I suppose option 1 is a slight favorite until we have a good reason. It's much easier to expand an API then to go through a deprecation/major-version cycle to remove it. :)

voith · 2018-09-20T05:56:53Z

I'm inclined towards option 1 aswell. I had always imagined this feature this way, but I only read the code in this PR yesterday. Although, this takes away alot of the convenience from the user, eth-tester IMO is a low level library and user trying to use it can be assumed to know a little about a genesis block. If this feature is documented well enough then I don't see any problem with having it.

@KPrasch If you make this change then I can promise to write the docs.

jMyles · 2018-09-20T10:30:19Z

This has turned into quite an endeavour!

I don't have any specific opinions on the underlying API change beyond simple hope and support, but I do want to echo @voith in pointing out that @KPrasch (a close friend and colleague of mine) has done some nice work here that improves the python ethereum ecosystem in a small but very meaningful way.

And also, thanks @voith for committing to writing the docs.

Whichever option is selected, the remaining work is non-trivial, so here's hoping for clear thoughts and an easy merge for everyone involved.

KPrasch · 2018-09-20T17:15:48Z

Awesome discussion and support, Thanks All! :-) 🛩️

With regard to creating genesis state mapping for accounts:

Cool, how about something like this API...

I like the Ideas presented above; Here is another Idea of how to handle injecting account states while avoiding usage of an empty dictionary to represent the default state:

override_accounts = (
  dict(balance=10**20),
  dict(nonce=2)
)
state = PyEVMBackend.generate_genesis_state(override_accounts, num_accounts=4)


#  Creates 4 accounts: two overridden states, and two default states
#  Raises ValueError if `num_accounts < len(override_accounts)`

This is very sensible, IMO:

state = PyEVMBackend.generate_genesis_state_duplicates(overrides=dict(balance=10**20), num_accounts=3)

Although, I can also imagine an even more dynamic usage of PyEVMBackend.generate_genesis_state to hand this case, too.

With regard to `PyEVMBackend.from_genesis`

For now, I think preserving the idea that you get exactly what the genesis file depicts is paramount. (Option 1) Web3.py provides sufficient tooling to handle the local keys.

It's much easier to expand an API then to go through a deprecation/major-version cycle to remove it. :)

It may be possible to build out more features using this tooling in the future. Also worth mentioning, back in Issue #88 there was discussion of restoring state from a snapshot, too.

And also, thanks @voith for committing to writing the docs. 📖

carver · 2018-09-20T18:36:15Z

If you make this change then I can promise to write the docs.

Great, thanks @voith !

Also worth mentioning, back in Issue #88 there was discussion of restoring state from a snapshot, too.

Yes, thanks for the callout. Snapshot & Restore belongs in a new issue, as Piper mentioned. (All of us can aim for more front-loaded discussion about the API this time :)) -- I just made #130 for it.

KPrasch has done some nice work here that improves the python ethereum ecosystem in a small but very meaningful way.

Totally agreed @jMyles !

Whichever option is selected, the remaining work is non-trivial, so here's hoping for clear thoughts and an easy merge for everyone involved.

Yup, I'm 99% of the way with you. The 1% -- I don't want to rush into a new public API (which has lasting maintenance effects) just because we want to merge in this contribution.

@KPrasch How about prefixing the PyEVMBackend methods with _ to further indicate that we may be iterating on them shortly? With that I would be 100% eager to merge:

_generate_genesis_params()
_generate_genesis_state()
_reset_to_genesis()

KPrasch · 2018-09-20T19:02:31Z

Yup, I'm 99% of the way with you. The 1% -- I don't want to rush into a new public API (which has lasting maintenance effects) just because we want to merge in this contribution.

No Rush, although I do think this may be well suited as multiple PRs!

How about prefixing the PyEVMBackend methods with _

Yes - mostly easy, except for the call in EthereumTester.reset_to_genesis

carver · 2018-09-20T20:18:05Z

I do think this may be well suited as multiple PRs!

For sure, #129 and #130 to track...

except for the call in EthereumTester.reset_to_genesis

Ah yes, right 👍

Thanks again for exploring this with us and being flexible!

KPrasch · 2018-09-20T20:35:39Z

Thank You!

voith · 2018-09-28T20:31:34Z

@KPrasch @jMyles Sorry for the silence. This got lost in the countless emails I get daily.

@KPrasch (a close friend and colleague of mine) has done some nice work here that improves the python ethereum ecosystem in a small but very meaningful way.

Agreed! I didn't have anything against this PR. Its just that while discussing, we came up with a different solution which solve a lot of use cases. Just didn't want the underlying API to be changed later, which would mean backwards incompatgibilty in the future.

And great that @carver merged this PR because I'm not sure how soon any of us will implement the newly discussed API.

I'll just wait for ethereum/py-evm#1299 to land.

KPrasch force-pushed the genesis_overrides branch from 7663be7 to 7bfe2a3 Compare August 19, 2018 08:03

KPrasch force-pushed the genesis_overrides branch from be40735 to 54036f3 Compare August 20, 2018 17:59

pipermerriam previously requested changes Aug 20, 2018

View reviewed changes

KPrasch changed the title ~~Accept genesis parameter overrides~~ [WIP] Accept genesis parameter overrides Aug 20, 2018

KPrasch force-pushed the genesis_overrides branch from dc39f4b to 05deb23 Compare August 29, 2018 09:27

KPrasch changed the title ~~[WIP] Accept genesis parameter overrides~~ Accept genesis parameter overrides Aug 29, 2018

voith reviewed Aug 29, 2018

View reviewed changes

KPrasch added 7 commits August 29, 2018 07:05

Provide facility to override genesis parameters; Reconcile reset_to_g…

92be1bc

…enesis methods across the codebase

Add tests for overridable genesis parameters.

4314629

Fix code style and convention

ee287cf

Improve Exception handing for genesis overrides; Use non-mutitive gen…

ee15807

…esis merge

Expose genesis overrides only at the backend level.

ecfd251

Refactor genesis parameter override test to use static method.

b57d189

Cleanup code style.

ef4bb2f

KPrasch force-pushed the genesis_overrides branch from 29b4d70 to ef4bb2f Compare August 29, 2018 14:06

carver requested changes Sep 17, 2018

View reviewed changes

KPrasch added 2 commits September 18, 2018 12:37

Accept optional genesis state overrides (lower-level)

3a969f6

Optional test account quantity when creating keypairs.

e2a1074

KPrasch force-pushed the genesis_overrides branch from 2e4aa24 to 1e67fd2 Compare September 18, 2018 19:50

KPrasch added 4 commits September 18, 2018 12:54

Use state overrides; Generate an equal number of keypairs and custom …

1690354

…account states at PyEVM init-time

Use dedicated utility function for genesis parameter merging

23e0b26

Fix a small found typo in an error message

d213465

Test customizable genesis parameter and state merging functionality; …

9ccb132

…Cleanup imports

Increased test accuracy and coverage for genesis parameter and state …

a199741

…overrides.

carver reviewed Sep 18, 2018

View reviewed changes

Clarify state override naming

e76eb55

KPrasch force-pushed the genesis_overrides branch from aaadd8f to e76eb55 Compare September 18, 2018 21:44

remove from_genesis_overrides classmethod

82972f4

KPrasch changed the title ~~Accept genesis parameter overrides~~ Accept genesis state and parameter overrides Sep 18, 2018

voith reviewed Sep 19, 2018

View reviewed changes

carver mentioned this pull request Sep 20, 2018

Better API for initializing with genesis parameters #129

Open

Mark PyEVM genesis override methods private

482fa29

KPrasch force-pushed the genesis_overrides branch from 9b801dc to 482fa29 Compare September 20, 2018 20:12

carver approved these changes Sep 20, 2018

View reviewed changes

carver merged commit a9ae913 into ethereum:master Sep 20, 2018

pipermerriam mentioned this pull request Sep 26, 2018

Documentation for specifying genesis state #132

Closed

KPrasch mentioned this pull request Oct 12, 2018

Update Readme with genesis overriding examples #136

Merged

Accept genesis state and parameter overrides #123

Accept genesis state and parameter overrides #123

Conversation

KPrasch commented Aug 19, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

pipermerriam commented Aug 20, 2018

KPrasch commented Aug 20, 2018

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

KPrasch commented Aug 20, 2018

KPrasch commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

KPrasch commented Sep 5, 2018

voith commented Sep 16, 2018

carver 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

carver Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

KPrasch commented Sep 18, 2018

carver 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

KPrasch commented Sep 18, 2018

carver commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voith commented Sep 19, 2018 • edited Loading

voith commented Sep 19, 2018

carver commented Sep 19, 2018

voith commented Sep 19, 2018

carver commented Sep 19, 2018

voith commented Sep 20, 2018

jMyles commented Sep 20, 2018

KPrasch commented Sep 20, 2018

With regard to creating genesis state mapping for accounts:

With regard to PyEVMBackend.from_genesis

carver commented Sep 20, 2018 • edited Loading

KPrasch commented Sep 20, 2018

carver commented Sep 20, 2018

KPrasch commented Sep 20, 2018

voith commented Sep 28, 2018

KPrasch commented Aug 19, 2018 •

edited

Loading

KPrasch Aug 29, 2018 •

edited

Loading

carver Sep 17, 2018 •

edited

Loading

voith commented Sep 19, 2018 •

edited

Loading

With regard to `PyEVMBackend.from_genesis`

carver commented Sep 20, 2018 •

edited

Loading