Support for custom genesis via EIP1085 #1299

pipermerriam · 2018-09-17T19:48:27Z

Continuation of #1247

fixes #722

What was wrong?

Trinity should be able to run custom chains beyond just mainnet and ropsten.

How was it fixed?

Now mainnet and ropsten are done in the same way that a custom chain would be, via an EIP1085 genesis file. There is now a new cli option --genesis which can be used to specify either the raw JSON string or a filesystem path to a JSON document.

Cute Animal Picture

pipermerriam · 2018-09-18T21:29:31Z

Don't bother looking at this. It's piling up into 3-4 separate PRs that need to be untangled.

voith · 2018-09-28T20:49:50Z

@pipermerriam from what I read, EIP1085 doesn't seem to be finalised.
Do you have any timeline for this PR to land. And even if it lands do you think the spec will remian unchanged? I'm asking because I'd like to integrate this into eth-teser as part of ethereum/eth-tester#129.

pipermerriam · 2018-10-01T23:53:43Z

@voith no idea on either (this PR or finalization of spec). I don't think that should block you however from moving forward with using it in eth-tester. Make sure the API is marked in some way as experimental and subject to breaking changes.

carver · 2018-10-04T18:26:00Z

trinity/assets/eip1085.schema.json

+          "pattern": "^0x([0-9a-fA-F]{2})+$"
+        },
+        "chainId": {
+          "title": "The chain ID",


"The EIP 155 chain ID" :) -- since everyone confuses the two.

veox · 2018-10-04T18:37:23Z

trinity/assets/eip1085.schema.json

+      }
+    },
+    "Accounts": {
+      "title": "Genesis block parameters",


Minor: looks like copy-paste left-over, here and below, in Account.

pipermerriam · 2018-10-11T21:26:17Z

Latest and greatest is pretty nice in some ways. It completely eliminates trinity.chains.mainnet and trinity.nodes.mainnet, making all chains completely generic (no special cased classes for mainnet/ropsten).

However, in doing this, we end up with errors when calls to chain.get_vm_configuration() happen because the dynamic chain classes aren't pickleable.

My current plan to remedy this is to make heavier use of the TrinityConfig object since it responsible for constructing the dynamic Chain classes, and to change some APIs to instead rely on getting the vm_configuration from that object (which is pickleable).

pipermerriam · 2018-10-18T19:09:12Z

More updates on this:

@carver and @cburgdorf

This is now yielding some interesting and fundamental flaws with how we currently do multiprocess communication, but I think it's a good thing. The concise issue is that with the introduction of custom genesis configurations, we end up with un-pickleable VM classes since they must be dynamically constructed based on the genesis configuration (this is primaily due to the dao fork, but even without this we'd have the same problem with POA networks since we'll have to inject the proper consensus engine).

That means all of Chain.get_vm_configuration(), Chain.get_vm, Chain.get_vm_class_for_block_number and probably a number of others can no longer be called across the multiprocessing boundary. I think the proper way to fix this is to do some or all of the following.

Remove the chain object from the things served up by the database process and require local chain instances in each process (thought the underlying database connection may still be across process boundaries)
Switch to RocksDB which allows for multiple database connections at which point we can potentially get rid of the database process entirely.

I see the potential for this to break some things, specifically surrounding our assumptions about where processing will happen and thus which calls are safe to make within the networking process without risking blocking too long and timing out with all of our peers.

Also, this further validates the event bus work that @cburgdorf has been doing with really explicitly and well defined communications which cross process boundaries.

carver · 2018-10-18T20:09:59Z

require local chain instances in each process

This sounds right to me. I believe I've done all the necessary work on Chain to make this fairly painless (no local state, all state depends on the database). The only necessary change was removing the canonical head attribute into the MiningChain.

carver · 2018-10-18T21:40:49Z

trinity/initialization.py

-            genesis_params=chain_config.genesis_params,
-            genesis_state=chain_config.genesis_state,
+            genesis_params=genesis_params,
+            genesis_state=genesis_state,


Maybe some of this logic that is just dumping in data from chain_config could be pushed inside chain_config, so the API could be:

chain = chain_config.launch_genesis(base_db)

If you like it

carver · 2018-10-18T21:43:41Z

tests/trinity/core/configuration/test_chain_config_object.py

+    chain = chain_config.full_chain_class.from_genesis(
+        AtomicDB(MemoryDB()),
+        chain_config.genesis_params.to_dict(),
+        {address: account.to_dict() for address, account in chain_config.genesis_state.items()}


pipermerriam · 2018-10-22T21:37:24Z

Working on rebasing and splitting this up but also pleased to report that a basic test of this code seems to be working right now.

veox · 2018-11-21T19:48:44Z

PR still missing Cute Animal Pic.

cburgdorf · 2018-11-21T19:51:32Z

@veox fixed!

carver

Obviously needs a rebase and/or squashing since there are dirty commits.

The only thing that might need more discussion is: what's happening in the transaction validator? At best, something is unclear, but it appears broken.

carver · 2018-11-21T19:58:11Z

tests/trinity/core/configuration/test_chain_config_object.py

+    chain = chain_config.initialize_chain(AtomicDB(MemoryDB()))
+
+    if network_id == MAINNET_NETWORK_ID:
+        assert chain_config.chain_id == MainnetChain.chain_id


Outside of the if block, you could test that the chain config ended up with the network ID it was created with, like:

assert chain_config.network_id == network_id

carver · 2018-11-21T20:02:23Z

tests/trinity/core/initialization/test_initialize_database.py



-def test_initialize_database(trinity_config, chaindb):
+@pytest.fixture
+def chaindb(trinity_config, base_db):


Looks like trinity_config arg can be dropped.

carver · 2018-11-21T20:13:01Z

trinity/plugins/builtin/json_rpc/plugin.py

-            chain = chain_class(header_db, peer_chain=event_bus_light_peer_chain)
-        else:
+            chain = chain_config.light_chain_class(header_db, peer_chain=event_bus_light_peer_chain)
+        elif trinity_config.is_full_mode:


I find this is_x_mode approach strange, in that it requires N is_x_mode attributes, one for each possible mode. I don't see a problem with the old sync_mode approach. If we're looking for more type-safety, sync_mode could be an Enum value instead of a str.

If we're looking for more type-safety, sync_mode could be an Enum value instead of a str.

With just two options, I don't feel strongly here but in general I agree that an Enum is nicer. Will change that.

@carver I found myself needing to import the enum/constant anywhere that I wanted to check sync mode, thus arose the creation of the is_x_mode. I don't feel strongly, but it was done to clean up the call sites.

Ok, I don't really have strong feelings at 2-3 modes, it just stands out as a little odd. By the time it gets to 5+ I would start to feel unhappy about it I guess.

carver · 2018-11-21T20:21:10Z

trinity/plugins/builtin/tx_pool/validators.py

+
+        if initial_tx_validation_block_number is None:
+            self._initial_tx_class = self._get_tx_class_for_block_number(
+                initial_tx_validation_block_number


This reads super strange to me. I think it's broken, but even if it's not, it's weird. If the block number is None then look up the transaction class by the number None? Maybe the if test is supposed to be reversed.

A couple layers down the stack, it looks like it breaks when trying to validate that None is a block number.

Looks like we might need a test for each of these if/else blocks, too.

I will add test for this. I think you are right with it being broken currently.

Done. Setup[ Robot| 2021 coming up/ Countdown.

cburgdorf · 2018-11-21T20:48:49Z

@carver Thanks for reviewing this!

Obviously needs a rebase and/or squashing since there are dirty commits.

Yep, will just squash this all together into one commit that adds custom genesis support

The only thing that might need more discussion is: what's happening in the transaction validator? At best, something is unclear, but it appears broken.

I think you are right. Will add tests for this (tomorrow)

pipermerriam

Mostly just suggestions on how this can be split up into some smaller pieces.

eth/chains/mainnet/constants.py

eth/chains/ropsten/constants.py

eth/tools/builder/chain/builders.py

tests/trinity/core/p2p-proto/test_v5_les_protocol_topics.py

tests/trinity/core/config/test_trinity_config_object.py

trinity/nodes/base.py

FrankSzendzielarz · 2018-11-21T23:18:47Z

Ping me please when custom genesis / config is ready to play with. I am adding Trinity to Hive.

cburgdorf · 2018-11-22T11:50:33Z

I didn't branch things out into multiple PRs but I sliced semantic commits that can be reviewed in isolation and should make it easier to follow.

I also added tests for the transaction validation stuff and corrected the functionality (it was indeed buggy).

I did not change the is_full_mode / is_light_mode stuff yet because the SYNC_LIGHT / SYNC_FULL constants are used at multiple places across the code base and if we refactor that to use an enum (I think we should) I would bloat this PR even more. I can send a follow up PR to refactor that.

@carver do you like to give this another review?

carver · 2018-11-22T17:01:09Z

I did not change the is_full_mode / is_light_mode stuff yet ... would bloat this PR even more

Yup, totally reasonable.

It's Thanksgiving, so I'll be mostly offline. I'll give a optimistic 👍 since you mentioned that this merge is needed to let master run a regular sync.

cburgdorf · 2018-11-22T17:05:57Z

@carver thanks! Happy Thanksgiving 🦃

cburgdorf · 2018-11-22T17:06:41Z

@FrankSzendzielarz This is now merged. Let me know if you run into any issues trying it out 🎉

pipermerriam added the PR state: WIP label Sep 17, 2018

pipermerriam force-pushed the piper/custom-genesis branch from 85fe86b to 43cddfd Compare September 18, 2018 17:13

pipermerriam mentioned this pull request Sep 18, 2018

Common genesis.json format scheme across all client implementations ethereum/EIPs#1085

Closed

pipermerriam force-pushed the piper/custom-genesis branch from 9b45f9b to f38cc52 Compare September 19, 2018 21:41

pipermerriam mentioned this pull request Sep 25, 2018

Better API for initializing with genesis parameters ethereum/eth-tester#129

Open

voith mentioned this pull request Sep 28, 2018

Accept genesis state and parameter overrides ethereum/eth-tester#123

Merged

pipermerriam mentioned this pull request Oct 2, 2018

Rename ChainConfig to TrinityConfig #1342

Merged

pipermerriam force-pushed the piper/custom-genesis branch from f38cc52 to c5be5cc Compare October 2, 2018 20:32

pipermerriam mentioned this pull request Oct 2, 2018

Piper/re organize trinity chain module #1347

Merged

pipermerriam force-pushed the piper/custom-genesis branch 5 times, most recently from d9a2f28 to bcce904 Compare October 3, 2018 21:19

carver reviewed Oct 4, 2018

View reviewed changes

veox reviewed Oct 4, 2018

View reviewed changes

pipermerriam force-pushed the piper/custom-genesis branch 2 times, most recently from 3e06920 to 4397e2b Compare October 18, 2018 18:40

carver reviewed Oct 18, 2018

View reviewed changes

pipermerriam mentioned this pull request Oct 19, 2018

BeaconChain #1413

Closed

pipermerriam force-pushed the piper/custom-genesis branch 3 times, most recently from 7f26a03 to 8b75601 Compare October 22, 2018 21:25

carver requested changes Nov 21, 2018

View reviewed changes

pipermerriam commented Nov 21, 2018

View reviewed changes

cburgdorf force-pushed the piper/custom-genesis branch 4 times, most recently from defffaf to 59f64a6 Compare November 22, 2018 11:45

cburgdorf added 6 commits November 22, 2018 16:56

Refactor some eth.typing types

3ebb8d9

Adjust type of Blocknumber constants

abd2d82

Move trinity config test file

8557504

Support custom genesis file via EIP 1085

91d9954

Add test for tx validation in tx pool

f957506

Make discovery v5 available in full mode

97342a3

cburgdorf mentioned this pull request Nov 22, 2018

Regular sync throws error #1504

Closed

cburgdorf force-pushed the piper/custom-genesis branch 2 times, most recently from 5402298 to 97342a3 Compare November 22, 2018 16:31

carver approved these changes Nov 22, 2018

View reviewed changes

cburgdorf removed the PR state: WIP label Nov 22, 2018

cburgdorf merged commit 2e674ce into ethereum:master Nov 22, 2018

veox mentioned this pull request Nov 24, 2018

cmd/puppeth: implement chainspec converters ethereum/go-ethereum#18172

Merged

pipermerriam deleted the piper/custom-genesis branch November 26, 2018 23:40

cburgdorf mentioned this pull request Dec 21, 2018

Expand docs to demonstrate how to use custom genesis files ethereum/trinity#83

Closed

pipermerriam mentioned this pull request Mar 23, 2019

Support for goerli testnet ethereum/trinity#72

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for custom genesis via EIP1085 #1299

Support for custom genesis via EIP1085 #1299

pipermerriam commented Sep 17, 2018 •

edited by cburgdorf

Loading

pipermerriam commented Sep 18, 2018

voith commented Sep 28, 2018

pipermerriam commented Oct 1, 2018

carver Oct 4, 2018

veox Oct 4, 2018

pipermerriam commented Oct 11, 2018

pipermerriam commented Oct 18, 2018 •

edited

Loading

carver commented Oct 18, 2018

carver Oct 18, 2018

carver Oct 18, 2018

pipermerriam commented Oct 22, 2018

veox commented Nov 21, 2018

cburgdorf commented Nov 21, 2018

carver left a comment

carver Nov 21, 2018

carver Nov 21, 2018

carver Nov 21, 2018

cburgdorf Nov 21, 2018

pipermerriam Nov 21, 2018

carver Nov 21, 2018

carver Nov 21, 2018

cburgdorf Nov 21, 2018

zaman10d Dec 31, 2020

cburgdorf commented Nov 21, 2018

pipermerriam left a comment

FrankSzendzielarz commented Nov 21, 2018

cburgdorf commented Nov 22, 2018

carver commented Nov 22, 2018

cburgdorf commented Nov 22, 2018

cburgdorf commented Nov 22, 2018

Support for custom genesis via EIP1085 #1299

Support for custom genesis via EIP1085 #1299

Conversation

pipermerriam commented Sep 17, 2018 • edited by cburgdorf Loading

What was wrong?

How was it fixed?

Cute Animal Picture

pipermerriam commented Sep 18, 2018

voith commented Sep 28, 2018

pipermerriam commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Oct 11, 2018

pipermerriam commented Oct 18, 2018 • edited Loading

carver commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Oct 22, 2018

veox commented Nov 21, 2018

cburgdorf commented Nov 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf commented Nov 21, 2018

pipermerriam left a comment

Choose a reason for hiding this comment

FrankSzendzielarz commented Nov 21, 2018

cburgdorf commented Nov 22, 2018

carver commented Nov 22, 2018

cburgdorf commented Nov 22, 2018

cburgdorf commented Nov 22, 2018

pipermerriam commented Sep 17, 2018 •

edited by cburgdorf

Loading

pipermerriam commented Oct 18, 2018 •

edited

Loading