VM, Common: Complex Genesis State tests

[cbrzn]

Changes on this PR:

• Genesis state with contract account stored are now being tested
• Type of AccountState has been created, making it part of GenesisState
• Documentation improved on customChain attribute in Common

Note that the GenesisState was previously being used only as string (not the complex object which is now AccountState) - Hence, not introducing breaking changes, since the complex object was not being used at all

Now, what I aim to do next, is point 3 from this comment #1708 (comment) and execute runTx first with a simple transfer tx and then a contract interaction.

If my understanding is correct, I think that I will need to first get the hash and state root through the client process (since in the Common package it still does not take into account the genesis state given to calculate these fields) and create a custom test data JSON, this way we instantiate a new Common object with this new JSON (+ custom state object) - Then, the execution should work, let's see how it goes :-)

[codecov]

Codecov Report

Merging #1757 (e89d16b) into master (da0c698) will increase coverage by 0.10%.
The diff coverage is n/a.

Flag	Coverage Δ
block	85.57% <ø> (ø)
blockchain	83.28% <ø> (ø)
client	75.23% <ø> (+0.09%)	⬆️
common	93.90% <ø> (ø)
devp2p	82.50% <ø> (+0.13%)	⬆️
ethash	90.76% <ø> (ø)
trie	86.18% <ø> (ø)
tx	89.94% <ø> (ø)
util	92.62% <ø> (ø)
vm	81.42% <ø> (+0.22%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[holgerd77]

Thanks, looks great already and super clean test setup. 🙂

Description of the next TODO steps also makes sense.

[holgerd77]

If my understanding is correct, I think that I will need to first get the hash and state root through the client process

Update: I am actually not sure if this is really needed for this. Just had a look again, the state root is only used for genesis block creation in the Block library. So you might want to try to run without in a first iteration.

Update 2: Uh uh uh. I am just realizing that we really just don't have this functionality or mechanism that the genesis state is initialized in the VM and a tx or so can then be executed with this state context. Ah. Interesting. 🙂

I guess this is something which can be added in a very similar way like the activatePrecompiles option we have in the VM already, so activateGenesisState I guess, which would then (also - similar to activatePrecompiles, see code: only when no state manager is given) initialize the state acounts + code.

Would value a confirmation from @jochem-brouwer on this, guess he might have some clear opinion on this.

I think such an option would generally be handy for various scenarios (e.g. also for our internal testing).

[holgerd77]

(but Cesar @cbrzn I guess you can continue with this also without an additional confirmation from Jochem or someone else, would be nice though)

[holgerd77]

Update 3 😋: ah, and you can then internally call the StateManager.generateGenesis() function for this - no need to reimplement this state addition logic - this should - theoretically - already be prepared for the initialization of the extended genesis state with code + storage.

[jochem-brouwer]

I'm not entirely sure what the question is here. Common cannot calculate state root. So it should be generated in VM? Isn't this functionality already around by importing the genesis block and then just reading the state root and verifying this is correct? Or is the problem that you only have a genesis state file and no genesis block file (so you cannot read state root?).

[cbrzn]

I'm not entirely sure what the question is here. Common cannot calculate state root. So it should be generated in VM? Isn't this functionality already around by importing the genesis block and then just reading the state root and verifying this is correct? Or is the problem that you only have a genesis state file and no genesis block file (so you cannot read state root?).

@jochem-brouwer

The problem we were having is that, when trying to run a transaction in the VM, we were not being able to somehow inject the genesisState attribute from common in the state context inside of the VM. In order to achieve this, we have added a flag called activateGenesisState in the VMOpts interface, which if true, will call the method generateCanonicalGenesis, hence, adding the genesis state in common in the stateManager of the VM.

What I am currently trying in this PR, more specifically in this file is to create a common instance with a given genesisState, create a new instance VM and try to execute transactions based on this data added in the genesis state.

I have a question tho, which I will dig through it later, but for now, if someone can give me insight would be amazing :-)

With this test, I am trying to execute a transaction to a contract previously added in the genesis state... In this contract (in solidity), a variable is defined, which to my understanding, is stored in the first slot of the storage attribute (which is 0x00..00). I am trying to execute the method that is defined in the contract, that (again, if I understand this correctly), returns the value of this stored variable. But in the object execResult, I am getting the bytecode of the contract as the returnValue , which I find weird.

It is worth mentioning that I am trying to trigger this retrieve method by encoding it into hex and passing it as the data argument. I am almost sure that I am doing something wrong because of my lack of holistic knowledge but I feel I am very close to achieve this :-)

[jochem-brouwer]

So we add an extra flag to signal the VM that there is a custom genesis state? Why can't we check if the provided common has a custom genesis state and then do this by default? People who would initialize common with a custom state would expect that VM also uses that state, and would take some time to figure out there is a flag.

Regarding your other questions: here is the gotcha.

You are right that the actual runtime code is returned. Why is this? This is because you have initiated the contract with the initcode. The initcode is the data you would send to your "create contract" transaction. What this does is (1) setup the storage fields to the desired values (e.g. set slot 0 to 4) and (2) return the runtime code. Since you put the initcode at the contract address, it thus returns the runtime bytecode, so your result is expected. Note that the code which you returned is not the same as the code which is at the contract (compare both). You are right that (if you would setup the contract using this runtime code) the slot 0 would contain 4.

What you should do is initiate the contract with code 0x6080604052600460005534801561001557600080fd5b5060b6806100246000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c80632e64cec114602d575b600080fd5b60336047565b604051603e9190605d565b60405180910390f35b60008054905090565b6057816076565b82525050565b6000602082019050607060008301846050565b92915050565b600081905091905056fea2646970667358221220338001095242a334ada78025237955fa36b6f2f895ea7f297b69af72f8bc7fd164736f6c63430008070033 (i.e. the returned code which you got earlier). Now what happens if you call retrieve()? Would you get 4?

No, you would not, as the contract has all storage slots set to 0 in the genesis. So, retrieve would return 0, not 4. To solve this, also put in genesis that at the contract at slot 0 you want value 4.

(Side note: it might be possible that if you the solidity optimizer it might realize that the value in the slot is impossible to change (it is a constant). In that case, solidity might hardcode the method to return 4. However, I checked, this is not the case, it actually reads from storage).

I also checked that your calldata is correct. Let me know if something is unclear or you want some more info 😄

@cbrzn

[holgerd77]

So we add an extra flag to signal the VM that there is a custom genesis state? Why can't we check if the provided common has a custom genesis state and then do this by default? People who would initialize common with a custom state would expect that VM also uses that state, and would take some time to figure out there is a flag.

Ah, that's an interesting idea, haven't thought about that from that perspective. Some aspects to that:

1. In case an external StateManager is provided, you definitely don't want to have that (so in the same way this is excluded for activatePrecompiles if a StateManager is provided). You would otherwise compromise the state the provided state manager is bringing in (e.g. from our client).

2. If people are initializing a VM, and then building up some state around this - maybe by directly accessing/using the internal StateManager from the VM - they will have some different total set of state entries (and therefore also a different state root) if we just activate this by default. In our tests we are actually using the internal VM StateManager for this all the time, e.g. here (just the first one I picked, not comparing the state root in the example code, could imagine that we are doing that as well somewhere though).

So I would definitely stick with a flag here. The question is if we might want to activate by default - and rather let people opt out. I would have a tendency to not do so though, I find this a bit risky not really knowing what people do around scenario 2., this comes too close to a breaking change for my taste.

[holgerd77]

(lot's of post-submission-edits in the last post, please read "on site" and not via email 😄)

[holgerd77]

Otherwise: love this conversation here! 😀 ❤️

[holgerd77]

@jochem-brouwer update: also, some room for a compromise: if you think this would really be highly convenient for users and would therefore push for having this activated, we can also add the flag being deactivated for now and switch over to activate along the breaking releases in develop and doing a note on the VM TODO.

(but also just a first thought, not completely sure if we should activate at all)

[cbrzn]

Thanks for the help @jochem-brouwer now the test is working as expected 😄

This is now ready for review @holgerd77

[ryanio]

I think instead of this additional note here, you can remove and add some more context below with a Pattern 3 (with complex genesis state, containing contract accounts and storage): for the next example below

Suggested change

	* Pattern 2 (with genesis state see {@link GenesisState} for format). Note that in {@link AccountState} there are two
	* accepted types. This allows to easily insert accounts in the genesis state
	* Pattern 2 (with genesis state see {@link GenesisState} for format):

[cbrzn]

Just updated it! Please let me know if you have any other feedback and I can implement it - Once everything is cool you can resolve this @ryanio :-D

[ryanio]

just a few review/nit comments, but otherwise great PR!! 👏

[holgerd77]

Can second that, this looks really great! 😀

There is an ethereum-tests folder update which sneaked in here, this needs to be removed. Otherwise I would say this is ready to go! 🎉

[cbrzn]

Same as the comment above, should I remove these non-related changes?

[ryanio]

This one is kind of annoying, I like the prettier format in the code sections especially, but could do without these table formatting. I wonder if we could turn off markdown table formatting specifically in our prettier root config file.

[holgerd77]

LGTM, thanks a lot Cesar! 🙂