Client: Save node key

[jochem-brouwer]

This PR ensures that we save the node key to disk. This is saved in the root directory of the current network. This way, we ensure that we will re-use the enode, such that we can quickly reconnect, or add our client as a bootnode. This would not be possible if it is not saved, since then our enode keeps changing every time we reboot the client.

[codecov]

Codecov Report

Merging #1067 (f1854b4) into master (e8099f1) will decrease coverage by 0.29%.
The diff coverage is 40.00%.

Flag	Coverage Δ
block	81.82% <ø> (+0.17%)	⬆️
blockchain	84.19% <ø> (ø)
client	83.79% <40.00%> (-0.97%)	⬇️
common	86.63% <ø> (ø)
devp2p	84.43% <ø> (ø)
ethash	82.08% <ø> (ø)
tx	84.21% <ø> (ø)
vm	81.67% <ø> (ø)

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

[ryanio]

It seems that this whole bootnodes option is already present - if you use the transports option you can add this as a comma separated list. I don't think I like that.

right, I don't think we should add a bootnodes option to the CLI, as transports can be used to define additional bootnodes, or otherwise the code added is redundant. Like so: --transports rlpx:bootnodes=bootnode1,bootnode2,bootnode3. This is already in the client readme, but under the libp2p key, so I could see why it could be confusing, we can add this additional documentation to show how to add bootnodes to rlpx.

You can see in #1027 I actually parse them as multiaddrs to pass around the client, which is a powerful type to use. The only thing is I haven't added yet is the enode ID (can be added as an extra key to multiaddr so e.g. /ip4/${ip}/tcp/${port}/enode/${id}) but that isn't being passed to devp2p at this point anyway. (it should, so it only connects to the peer if it has the same id, otherwise could be a compromised node). In my newly added parseMultiaddrs function it can parse the id@ip:port format so we don't need that additional code added in config.ts for parsing.

The consistent key is great though, and I can add that to my newly updated libp2pserver file as well. Maybe we can create a helper func so the code isn't duplicated from rlpxserver.

[jochem-brouwer]

I think you are right. I will check if this is possible, since it might be that this string parser might choke upon the address string format.

[jochem-brouwer]

Just checked:

npm run client:start -- --transports rlpx:bootnodes=d860a01f9722d78051619d1e2351aba3f43f943f6f00718d1b9baa4101932a1f5011f16bb2b1bb35db20d6fe28fa0bf09636d26a87d31de9ec6203eeedb1f666@18.138.108.67,22a8232c3abc76a16ae9d6c3b164f98775fe226f0917b0ca871128a74a8e9630b458460865bab457221f1d448dd9791d24c4e5d88786180ac185df813a68d4de@3.209.45.79:30303

(First two bootnodes from mainnet Common). This parses the bootnodes wrong:

{
  name: 'rlpx',
  options: {
    bootnodes: 'd860a01f9722d78051619d1e2351aba3f43f943f6f00718d1b9baa4101932a1f5011f16bb2b1bb35db20d6fe28fa0bf09636d26a87d31de9ec6203eeedb1f666@18.138.108.67',
    '22a8232c3abc76a16ae9d6c3b164f98775fe226f0917b0ca871128a74a8e9630b458460865bab457221f1d448dd9791d24c4e5d88786180ac185df813a68d4de@3.209.45.79:30303': undefined
  }
}

[jochem-brouwer]

I can't come up with a consistent format scheme here, will open an issue.

[holgerd77]

@jochem-brouwer can you give this another look at some PIT?

[jochem-brouwer]

Will make this one merge-ready ASAP @holgerd77 😄

[holgerd77]

@jochem-brouwer can you do an update here?

[holgerd77]

(if time permits, not super urgent, but would be nice to get this finished)

[jochem-brouwer]

@holgerd77 Yep, sorry forgot about this one. Will probably do today, but by end of this weekend the latest.

[holgerd77]

No problem. 😄

[jochem-brouwer]

Just rebased this one. If we want to introduce this node key into a new database (which we currently don't have) I am facing the problem where I should put the database. The logical place for this database would be to be in Config. Would it make sense to add a global config DB here? This would depend on the data dir, so we don't use the same config for e.g. mainnet and rinkeby.

[holgerd77]

@jochem-brouwer yeah, I think it makes sense to add a global config DB and the Config class should be good place I guess. 😄

This would depend on the data dir, so we don't use the same config for e.g. mainnet and rinkeby.

I am not sure here, we'll likely have both global configuration parameters as well as chain-specific ones. Would it be a way to simplify and use one DB for both, and then prefix keys, so e.g. general:config_key1 (or global or config), rinkeby:config_key2, or similar?

[jochem-brouwer]

If we want to have global config and also chain-specific config, would it then make sense to introduce a global database and also a chain-specific one?

[holgerd77]

For me having 5 databases for 20 config parameters sound a bit overblown, but I'm also really no expert here. Would be happy on further voices.

[ryanio]

I rebased this and added a client config leveldb in the network directory for getting and setting the client key.

[holgerd77]

Nice, LGTM! 😄 Will directly use this to more comfortably debug #1147, hehe 😛 (really annoying to always copy over a new enode ID on every new test run)

[holgerd77]

Won't make this a blocker, but this shouldn't be necessary since datadir has a local CLI-specific default (first was afraid that this would actually change the default, but this is not the case)? 🤔

[holgerd77]

Ah, and also tested this locally and can confirm that this works, client key / network URL is perceived.

[holgerd77]

I wonder though if we want to make the name of this option a bit more expressive?

[holgerd77]

@ryanio on a closer look I think this is logically wrong, the client key from the DB shouldn't be re-passed in to Config, this makes not much sense. I stumbled upon this since this is currently breaking when using a custom datadir with "IO error: datadir-dev2/mainnet/config/LOCK: No such file or directory".

When trying to fix this (with ensureDirSync in cli.ts) this get's unnecessarily complex since one is still acting in a static context where Config is not existing yet and one would therefore need to manually construct the directory paths.

Will rework this a bit along #1147.

[holgerd77]

@ryanio Update: ok, I am slowly getting why you are doing this, seems this has again this leveldb creation problem (or at least I am stumbling atm upon that I don't want to introduce the fs-extra dependency to config). So I will just give this a hot-fix right now on the mentioned PR.

I would still assume that this is structurally not correct and we should give this some additional thought (it just shouldn't be necessary to have access to the configuration database to actually create a Config object). Maybe we can rework on the addition of the next config option or something (or alternatively directly doing now).

[ryanio]

@holgerd77 ah thanks, yeah I had some challenges with the async nature of saving or getting the client key from the db, so I had to move the async part out of the config/server instantiation.