Where to put VM?

[jochem-brouwer]

We want to integrate the VM in the client. Where do we want to put it? We discussed putting this in config, so people can use their custom implementation of the VM. The problem is, though, that in the constructor of VM we need access to Common (available in config) and Blockchain (created after we need config).

Call stack is Node -> Full/Light EthereumService -> EthereumService

I think the first step here would be to move the blockchain to the config aswell. Note that although EthereumService accepts a options.chain parameter, this is never passed (from FullEthereumService).

Another idea here would be to instead of passing the objects into the config, we could also pass the constructor function to the config. In that case, we only call the constructor of that specific class when we need it (and don't have to create an object if we don't need it, such as creating a VM in the LightClient). But this is a rather abstract idea, worth thinking about though.

[holgerd77]

Ah, just read this after commenting on #187. Just had a look at the Blockchain initialization, should be no problem to move this to config, db should also be moved over there along, this is currently a separate option in node.ts.

I would suggest the following:

• Move db to Config and initialize there instead of in bin/cli.ts (db: level(syncDataDir)), keep internally for now (so no option, but config.db)
• Move blockchain to Config keep it internally for now (so no option, but config.blockchain)
• Move vm to Config and expose as an option

[holgerd77]

Ah, if we don't use runBlockchain you don't need to pass the blockchain object to the VM, so you can just move the VM to Config and leave everything else

[jochem-brouwer]

I will implement your suggestion. Note that it is critical that the VM blockchain points to the blockchain which we need for BLOCKHASH, if we pass a different blockchain then it is possible that either the block does not exist, or it is the wrong block.

[jochem-brouwer]

Do we want to keep the chain and db options available in EthereumServiceOptions? They seem redundant now that these options are put in the config, we can read from there.

However, this option is used in the tests - therefore we should make expose blockchain and db available as options in config. Thoughts?

[jochem-brouwer]

Right I keep finding these issues when moving stuff to config. In test/util.ts we create two Config objects. This is problematic because these try to open two databases on the same sync dir, which is not possible on leveldb.

[holgerd77]

Just as a general answer on first round, independent from how we decide here: options added to config should then definitely removed from dedicated class files (like EthereumService in this case), this is one of the main reasons for the Config introduction to streamline here.

test/util.ts doesn't exist for me (?) but apart from that I would very much assume that this should be easily solvable by just passing two different datadir directory values to the different config files? 🤔

[jochem-brouwer]

Ah sorry I meant test/integration/util.ts.

What do you mean here by "options added to config"? Can you give an example? Are these the options like the chain and db in EthereumServiceOptions? What options are also added in config?

[holgerd77]

With "options added to config" I mean everything which is exposed by the config instance passed from class to class. These properties should then always be accessed through e.g. config.syncmode and not passed from class to class any more which would be redundant.

Most of these options are also exposed as the public API by being in the interface - so e.g. syncmode - others like servers are then derived from the options passed and then created internally in Config, but are nevertheless client-internally available for config based consumption.

[holgerd77]

Some additional note on "public API" - if this might not be clear on first read: the Node class will become at some point - at least that's my current view on this - become (or is already) the entrypoint to the client with the public API being encapsulated by the Config class. This will be how people will use the client programatically.

That's why I wouldn't regard the current Node class interface as finalized:

export interface NodeOptions {
  /* Client configuration */
  config: Config

  /* Blockchain database (default: null) */
  db?: LevelUp

  /* List of bootnodes to use for discovery */
  bootnodes?: BootnodeLike[]

  /* List of supported clients */
  clientFilter?: string[]

  /* How often to discover new peers */
  refreshInterval?: number
}

All the other options apart from config listed here seem a bit misplaced and should from my PoV at some point move into Config, these are very much options completely on the level like minPeers, e.g. refreshInterval. I would also count db to something which doesn't really belong here - to refer to our previous discussion - but also not sure yet how to relate this to the datadir option and what makes sense here to expose.

Also one important distinction: the bin/cli.ts script can be a bit regarded as an "outer" script how WE use this API defined by Node. So from some point of view bin/cli.ts doesn't belong to the client (everything in lib) any more. Or at least, the interface between cli.ts and node.ts is very central and should be crafted with care. 😄

Just as some additional insight on my thinking, nothing to really consider or act on. 😋