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

Common: Support Geth Config #1708

Closed
jochem-brouwer opened this issue Feb 9, 2022 · 14 comments
Closed

Common: Support Geth Config #1708

jochem-brouwer opened this issue Feb 9, 2022 · 14 comments

Comments

@jochem-brouwer
Copy link
Member

Some chains, like MATIC/Polygon, initialize certain contracts on the Genesis block. This is not supported currently in Common, we can only add (?) balance to accounts. To import these chains, genesis should support setting code/storage/nonces of accounts as well.

@jochem-brouwer
Copy link
Member Author

Related: #1696

@holgerd77 holgerd77 changed the title Common: add support for genesis to add code/storage/nonces to genesis accounts Common: add support for genesis to add code/storage/nonces to genesis accounts / Support Geth Config Feb 11, 2022
@holgerd77
Copy link
Member

I will expand this issue a bit with adding "Support Geth Config (in Common)" to the title. This is very related since in the Client we already support this functionality along the Geth genesis.json import/parse logic and it would be good to not double the code base here but optimally unify in Common.

Furthermore it would generally be good if Common would be able to itself read in Geth config files since this would expand on the use cases, e.g. to have this kind of configuration files as a basis for VM runs.

I can vaguely remember that this was in discussion already when we were implementing, and then we had some blockers with prevented this from being implementable in an easy way. Also a vague remembrance: I think it had something to do that - atm we have the stateRoot and the hash in the Common chain file genesis definition - see e.g. mainnet.json, which are both not present in the Geth genesis.json files and which would need to be recomputed which was not easily possible. I wonder if this would be easier (to remove both of those values) along the upcoming breaking releases? 🤔

Not sure, can't recall the whole chain of complexity here any more, maybe @acolytec3 Andrew can shine some light here since I think he did the implementation on this and stumbled upon these kind of things.

@acolytec3
Copy link
Contributor

I'd have to do some exploratory coding but it should be feasible to compute the stateroot and hash if not provided in a genesis file. That's what we do in the client to parse the geth genesis file. The stateroot is calculated here. As I recall, the issue with putting this in common was the concern over adding the trie dependency to common.

@holgerd77
Copy link
Member

I'd have to do some exploratory coding but it should be feasible to compute the stateroot and hash if not provided in a genesis file. That's what we do in the client to parse the geth genesis file. The stateroot is calculated here. As I recall, the issue with putting this in common was the concern over adding the trie dependency to common.

Ah yes, thanks, remember a bit more clearly. I guess we need to have a closer look where stateRoot and hash are actually used directly and if/how this can be easily replaced.

Can't say for sure, but I vaguely remember that this was a bit tricky (stateRoot being used in a below-VM/StateManager based library or something like that).

@holgerd77
Copy link
Member

Update: ah, stateRoot is used in Block for initializing a genesis header (with: initWithGenesisHeader) if the stateRoot field is not present.

I guess strictly speaking this is somewhat of a design flaw - I would call this an "implicit circular dependency" - since we are expecting here (in the Block library) some state context (the calculated state root) while - theoretically - this doesn't belong to Block (block should not need to have some notion of state) - and rather to the Blockchain library.

Some meta note: I often thought already that we likely have several of these very, very subtle design (structural) flaws in our libraries which then makes it harder to realize on something else.

So in this case the hard (but maybe also the correct) way would be: remove/replace/refactor this option.

This state root dependency just shouldn't be there at that place.

@holgerd77
Copy link
Member

holgerd77 commented Feb 11, 2022

But all this might not be practical.

So one way to circumvent this would be to always require the triple of

gethGenesis: {
  genesisJSON: file,
  stateRoot: string,
  hash: string
}

As an input if someone wants to run Common with a custom genesis file in Geth format. Then we leave it to the user to provide this.

(I guess we already were at this place if I recall correctly. 😋)

Along we could provide a short library-independent script (or two) for creating the hash and stateRoot if necessary (or - maybe also a way: add some dedicated helper function on this in a fitting upstream library (Blockchain? Client? VM? No idea atm.).

Still seems like a feasible way to go to me.

@acolytec3
Copy link
Contributor

Along we could provide a short library-independent script (or two) for creating the hash and stateRoot if necessary (or - maybe also a way: add some dedicated helper function on this in a fitting upstream library (Blockchain? Client? VM? No idea atm.).

I dug back through the old issues/PRs around this because I thought I had written just such a script at one point but couldn't come up with anything. I can't find the conversation either but my recollection is that at least when I was approaching it, having an independent script always required installing the trie library to come up with the stateRoot. For someone wanting to run a custom chain, that doesn't seem problematic, right?

@holgerd77
Copy link
Member

Ok, I had a closer look here and discovered that we already have this extended genesis format accepting code and storage in addition to just initialize with EOAs just holding account balances. This just has never been tested (nor documented 😋).

So I guess on a first round we should forget about the Geth config support and limit on the following tasks:

  1. Add an additional test case in Common here in the customChains.spec.ts file which uses the extended genesis format and also passes in code and storage
  2. If this works 😋: add some additional description of the use case to the customChains constructor option (Pattern 3, in this case likely not referencing a file but directly giving a short description of the data format)
  3. Add two additional test cases to the VM:
    3.1 A "simple" one with a Common using the customChains option to initialize with a simple "only accounts" custom genesis state and then trigger a simple money transfer on these accounts
    3.2 A more sophisticated one with a Common initialized with an extended genesis state also with code, eventually storage, and then triggering a simple execution on that code (whehew! 🙂)
    3.3 Additional note: I would be a fan of doing both in a new tests/api/customChains.spec.ts file and also move the two general test cases from tests/api/index.ts on this over to this new file.
  4. I guess we also want a dedicated section "Custom Chain VM Execution" in the VM Readme somewhere below here and/or a fitting example on this 🙂

This work should be done towards master.

@cbrzn
Copy link
Contributor

cbrzn commented Feb 23, 2022

After digging through this for a while and reading your discussions in different issues/threads, I have come to some conclusions:

  • Currently, when passing a genesis state into the customChain object, this genesis is not being used at all and it's just being called in the genesisState() method
  • If I understand this correctly, in the geth file parser, we are generating the state hash, as previously mentioned by Andrew; and then creating the block header with this new hash, that's how the genesis object it's created when parsing geth params
  • When using the common library to create a custom chain with genesis state, this state root is not being generated - Because we are creating the genesis object from the received parameters of the chain, not taking into account the genesis state.
  • This means that when receiving a genesis state, we need to:
    - Create the state root hash using Trie library
    - Create block header based on this data with the new state root hash
    - Create genesis object with hash and state root hash of header

I think that this can be achieved by doing the following:

  • Move createGenesisStateTrie function outside of client and make it a helper of the Common library (I saw in your comments you don't think it's a good thing to add the Trie dependency to Common so I am open to suggestions here, but I think we should def make a helper script that parses any state object and converts a hash of it)
  • Standardize the parameters expected and returned in this function, meaning that it should not expect any, but instead an explicit type to make it easier to interact with.
  • The GenesisState can be improved to something like (Making it easier to create a genesis state based on a user-friendly interface):
type GenesisState = {
  [address: string] = {
    code: string;
    balance: string;
    storage: Record<string, string>
  }
}
  • Implement this helper scripts in geth parser & when implementing a custom chain (with custom genesis state) in the common library

Note: Sorry if something does not make sense, this is based on my current understanding of things :-)

@holgerd77
Copy link
Member

Hi @cbrzn, great, thanks for this write-up, yes, I think you are describing the situation pretty acurately. 😄

Yes, adding the Trie library to Common is not an option. We generally want to make Common rather leaner than heavier, since this is connect also to the lower-level objects like a tx and these packages then need to draw in all the additional dependencies which is basically "only" for some configuration. I was rather also already thinking along the lines of getting all genesis functionality completely out of Common, the genesis stuff is (mainly) used in Blockchain and upwards, so I was temporarily thinking along the lines of moving all the genesis state json files to the Blockchain class on the breaking releases, this would need to go along with some substantial refactoring along various lines and I still need to give this some thinking, eventually together with @jochem-brouwer along the StateManager standalone-package-refactor.

My idea for this round (this should be work towards master in a non-breaking way) - to keep this manageable - would be to not add a dedicated script or something but simply make sure when an @ethereumjs/client instance is started with a custom geth genesis chain file - that stateRoot and genesis block hash is put out to the client console log. There all the necessary code is already there and then we just need to implement the API from above in Common:

gethGenesis: {
  genesisJSON: file,
  stateRoot: string,
  hash: string
}

and point people who want to use this to the client and describe in Common README and/or code docs that they should do a short npm i @ethereumjs/client and run the genesis file (they should have at this point) on the client and should read the values out of the log.

Thing is: this geth-genesis-file-to-common is really rather just a nice-to-have feature right now and I wouldn't want to overblow this task and make it a 2-week-long thing. The more important thing is this first "add support for genesis to add code/storage/nonces to genesis accounts" part of this issue (so this would be to add the tests I described in the comment from above).

You are very much invited to improve the typing on these things, it just should remain - in the scope of this task - backwards-compatible.

@cbrzn
Copy link
Contributor

cbrzn commented Mar 27, 2022

Now that we have the complex genesis state - I think the next steps to tackle support geth config can be something like:

  • When running the client with a genesis geth file, show block hash and state root hash with console.log
  • Add a new type GethGenesisState which will have the proposed interface from your previous comment
  • Extend the customChains attribute in Common constructor to also support the new state; i.e: _customChains: IChain[] | [IChain, GenesisState | GethGenesisState][]
  • Update the method setChain to check if the state in customChain has the attributes hash and stateRoot you can see in this PR how I see it

This game plan is based on the previous PR mentioned but only focuses to support geth file config in common. This will allow us to set the genesis object with the desired state provided from the hash and state root

@holgerd77
Copy link
Member

Now that we have the complex genesis state - I think the next steps to tackle support geth config can be something like...

That sounds all good, thanks for the write-up! 😄

When running the client with a genesis geth file, show block hash and state root hash with console.log

Just a small nit: this can (should, I guess) just be part of the normal INFO log output (something like "Geth config file loaded chain=... id=... stateRoot =... hash=...) (just a very quick write-up with the broader notion: output some useful information here 🙂) rather than use console.log for this.

@holgerd77 holgerd77 changed the title Common: add support for genesis to add code/storage/nonces to genesis accounts / Support Geth Config Common: Support Geth Config Jun 15, 2022
@holgerd77
Copy link
Member

Update: there has been a substantial rework of the genesis-related functionality - also in Common - taken place in #1916 for v6 - the main remaining tasks would be to support Geth configuration directly in Common (which should now easier be doable I guess with hash and stateRoot removed which were the main blockers).

This is rather a mid-term task though for after the breaking releases are done.

@holgerd77
Copy link
Member

This has been addressed in #2300, will close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants