WIP: Use private genesis config for private network #1247

cpurta · 2018-09-03T18:49:18Z

Will resolve #722 when merged.

What was wrong?

Currently trinity does not allow for the use of a private chain by specifying a custom genesis block. Trinity only allows the use of the mainnet and ropsten test-net.

How was it fixed?

Allow trinity to use a custom genesis configuration to allow for private
networks to be specified. The changes specify that a genesis flag be
supplied with a valid filepath to the genesis config. It will also
require that a datadir flag is provided.

If using the custom genesis config if it does not contain the required
parameters trinity will not continue to try and create a new network.
Testing is still yet to be implemented.

Allow trinity to use a custom genesis configuration to allow for private networks to be specified. The changes specify that a genesis flag be supplied with a valid filepath to the genesis config. It will also require that a datadir flag is provided. If using the custom genesis config if it does not contain the required parameters trinity will not continue to try and create a new network. Testing is still yet to be implemented.

pipermerriam

Some basic comments. 👍

I believe this will be a good candidate for the integration test suite when it is done.

pipermerriam · 2018-09-03T20:06:29Z

trinity/chains/__init__.py

@@ -56,6 +65,50 @@
 )


+def validate_genesis(chain_config: ChainConfig) -> (BlockHeader, int):


Can we change this to validate_EIPXXX_genesis (with the XXX replaced by the EIP number), as well as maybe linking to the EIP in the docstring for easy reference.

pipermerriam · 2018-09-03T20:07:23Z

trinity/chains/__init__.py

+    Checks the genesis configuration file provided and ensures that it is valid.
+    """
+    if not os.path.exists(chain_config.genesis):
+        raise MissingPath(


I think the built-in FileNotFoundError would be sufficient here.

pipermerriam · 2018-09-03T20:08:08Z

trinity/chains/__init__.py

+
+    with open(chain_config.genesis, 'r') as genesis_config:
+        raw_genesis = genesis_config.read()
+        genesis = json.load(raw_genesis)


These two lines should be replaced with genesis = json.load(genesis_config). No need to read() the file object.

pipermerriam · 2018-09-03T20:08:26Z

trinity/chains/__init__.py

+        raw_genesis = genesis_config.read()
+        genesis = json.load(raw_genesis)
+
+        if not 'chainId' in genesis.keys():


Once we've parsed the JSON, we can exit the with block above, dedenting these lines a level.

pipermerriam · 2018-09-03T20:09:39Z

trinity/chains/__init__.py

+        if not 'extraData' in genesis.keys():
+            raise ValidationError("genesis config missing required 'extraData'")
+
+        return BlockHeader(


Our general pattern is that validate_ functions don't return data, so this should likely be re-written to three independent things.

load the JSON

validate the JSON

extraction (block header, chain configuration, etc)

…genesis-config

pipermerriam · 2018-09-04T20:39:57Z

@cpurta let me know when you'd like me to take another look.

cpurta · 2018-09-04T21:13:20Z

@pipermerriam feel free to take another look. Most recent commits have been just trying to get tests to pass.

pipermerriam

the ChainConfig.genesis and ChainConfig.genesis_header properties will be None when running a non-private chain which is not idea. It would be good to have those properties return the correct data for pre-configured chains as well.

I assume that there is still pending work on this as I don't see anywhere that the fork configuration is being extracted from the genesis data.

pipermerriam · 2018-09-04T21:16:35Z

trinity/chains/__init__.py

+    if 'extraData' not in genesis.keys():
+        raise ValidationError("genesis config missing required 'extraData'")
+
+    return True


Most of our other validation functions don't have return values. They either implicitly return None or raise a validation error. Take a look at eth/validation.py for examples of this.

pipermerriam · 2018-09-04T21:19:20Z

trinity/chains/__init__.py

+    vm_configuration = ()  # type: Tuple[Tuple[int, Type[BaseVM]], ...]
+
+
+class PrivateChain(BasePrivateChain, Chain):


Private carries meaning that I don't think applies to all use cases of this. Maybe CustomChain? cc @carver if you have any suggestions.

pipermerriam · 2018-09-04T21:21:00Z

trinity/config.py

+        """
+        Return the path of the genesis configuration file.
+        """
+        return self.genesis


This should access self._genesis

pipermerriam · 2018-09-04T21:21:12Z

trinity/config.py

+        """
+        Return the genesis block header parsed from the genesis configuration file.
+        """
+        return self.genesis_header


This should be accessing self._genesis_header

pipermerriam · 2018-09-04T21:23:50Z

trinity/chains/__init__.py

@@ -56,6 +70,49 @@
 )


+def is_valid_genesis_header(genesis: Dict) -> bool:


With the removal of the bool return, this should be renamed something like validate_eip1085_genesis

Moved the helper functions to parse the genesis config and generate the header to the utils chains.py file. Seems like a better place for them and no longer having issues with importing the functions at runtime. Added the data-dir to be a requirement when the genesis flag is provided. Correctly build the genesis header to the correct types.

Persist all of the vm configurations from the genesis config file specified to the custom chain. Then allow for the chain to use either full or light syncing with other nodes in the network.

cpurta · 2018-09-06T19:59:19Z

Got this to a state to where it is using the genesis config correctly and is now running using that config. But it seems to be wanting to connect to the discovery peers to sync with them. Also will be addresses some of the failing CI tests.

…n nodes

cpurta · 2018-09-06T23:53:47Z

@pipermerriam I believe that this could use another review before I address the issues in CI.

pipermerriam · 2018-09-12T15:45:56Z

@cpurta 👀

cpurta · 2018-09-17T15:29:43Z

@pipermerriam any comments on the changes? Going to try and tackle the CI issues today.

pipermerriam · 2018-09-17T18:22:26Z

@cpurta I started looking but got pulled away last time. Going to try and close this out this week. Likely late today or tomorrow before I get to reviewing this.

cpurta · 2018-09-17T18:25:14Z

@pipermerriam appreciate the response. I know too well how that goes 😄. I will be addressing the CI issues in the meantime.

pipermerriam · 2018-09-17T18:26:42Z

eth/chains/base.py

@@ -125,6 +125,10 @@ class BaseChain(Configurable, ABC):
    def __init__(self) -> None:
        raise NotImplementedError("Chain classes must implement this method")

+    @property
+    def vm_configuration(self) -> Tuple[Tuple[int, Type['BaseVM']], ...]:
+        return self.vm_configuration


This is going to cause infinite recursion.

Also, I believe we already have a get_vm_configuration method that you can use.

Remove the vm_configuration as a class level property from the BaseChain class. This is due to the potential for infinite recursion and is causing errors in CI. Also addressing some the errors from the lint test in CI.

pipermerriam · 2018-09-17T19:47:45Z

@cpurta I'm going to take this PR over to get it across the line and merged. thanks for getting it this far 😹

cpurta changed the title ~~Use private genesis cofig for private network~~ WIP: Use private genesis config for private network Sep 3, 2018

pipermerriam reviewed Sep 3, 2018

View reviewed changes

Chris Purta added 7 commits September 4, 2018 08:06

Refactor function that parses and checks the genesis header

918431e

Move get_EIP1085_header to utils.chain.py

1999573

Fix linting errors and import BlockHeader

4db291c

Merge branch 'master' of https://github.com/ethereum/py-evm into add-…

36c5b54

…genesis-config

Fix unused/missing imports for trinity/chains

b523f3c

Fix type annotations for linting errors

89d93cc

Import Tuple to fix linting error

811124e

Import BaseVM for CI linting test

9d1db44

pipermerriam reviewed Sep 4, 2018

View reviewed changes

Chris Purta added 4 commits September 6, 2018 08:16

Persist vm config from genesis, create full and light custom nodes

981619c

Persist all of the vm configurations from the genesis config file specified to the custom chain. Then allow for the chain to use either full or light syncing with other nodes in the network.

Fix linting errors from CI

5812ab7

Fix last linting error from CI

8b40fc1

Chris Purta added 2 commits September 6, 2018 13:26

Stop custom chain node from using hard-coded discovery mainnet/ropste…

73582ef

…n nodes

Add default chain headers and vm_configs for main/ropsten chain config

9e09933

Chris Purta added 3 commits September 12, 2018 08:25

Resolve merge conflicts from master

ba5d7b6

Fix merge conflict

b465d8c

Add missing propery wrapper to trinity_root_dir

279bf79

pipermerriam reviewed Sep 17, 2018

View reviewed changes

Chris Purta added 3 commits September 17, 2018 12:09

Address PR comments fix linting errors

0afdcf7

Remove the vm_configuration as a class level property from the BaseChain class. This is due to the potential for infinite recursion and is causing errors in CI. Also addressing some the errors from the lint test in CI.

Remove unused imports and add missing trailing commas

899baff

Add back the type for vm_configuration in BaseCustomChain

85fe86b

pipermerriam closed this Sep 17, 2018

pipermerriam mentioned this pull request Sep 17, 2018

Support for custom genesis via EIP1085 #1299

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Use private genesis config for private network #1247

WIP: Use private genesis config for private network #1247

cpurta commented Sep 3, 2018

pipermerriam left a comment

pipermerriam Sep 3, 2018

pipermerriam Sep 3, 2018

pipermerriam Sep 3, 2018

pipermerriam Sep 3, 2018

pipermerriam Sep 3, 2018

pipermerriam commented Sep 4, 2018

cpurta commented Sep 4, 2018

pipermerriam left a comment

pipermerriam Sep 4, 2018

pipermerriam Sep 4, 2018

pipermerriam Sep 4, 2018

pipermerriam Sep 4, 2018

pipermerriam Sep 4, 2018

cpurta commented Sep 6, 2018

cpurta commented Sep 6, 2018

pipermerriam commented Sep 12, 2018

cpurta commented Sep 17, 2018

pipermerriam commented Sep 17, 2018

cpurta commented Sep 17, 2018

pipermerriam Sep 17, 2018

pipermerriam Sep 17, 2018

pipermerriam commented Sep 17, 2018

		@@ -56,6 +65,50 @@
		)


		def validate_genesis(chain_config: ChainConfig) -> (BlockHeader, int):

		vm_configuration = () # type: Tuple[Tuple[int, Type[BaseVM]], ...]


		class PrivateChain(BasePrivateChain, Chain):

		@@ -56,6 +70,49 @@
		)


		def is_valid_genesis_header(genesis: Dict) -> bool:

WIP: Use private genesis config for private network #1247

WIP: Use private genesis config for private network #1247

Conversation

cpurta commented Sep 3, 2018

What was wrong?

How was it fixed?

pipermerriam 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

pipermerriam commented Sep 4, 2018

cpurta commented Sep 4, 2018

pipermerriam 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

cpurta commented Sep 6, 2018

cpurta commented Sep 6, 2018

pipermerriam commented Sep 12, 2018

cpurta commented Sep 17, 2018

pipermerriam commented Sep 17, 2018

cpurta commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Sep 17, 2018