Implement more of the KeyStore spec and integrate it in the beacon node #1093
Conversation
zah
changed the title
zah
changed the title
zah
force-pushed
the
keystores
branch
2 times, most recently
from
02930c9 to
bdd2cb8
Compare
| h.update zh[i-1] | ||
| zh.add nodehash | ||
| zh | ||
|
|
||
| type SparseMerkleTree*[Depth: static int] = object |
There was a problem hiding this comment.
a brave soul would use a HashList here which would likely speed up generation considerably (the code looked like it was recomputing lots of stuff unnecessarily) - not that it really matters, unless you're generating lots of keys.
| @@ -106,6 +105,14 @@ type | |||
| abbr: "v" | |||
| name: "validator" }: seq[ValidatorKeyPath] | |||
|
|
|||
| validatorsDirFlag* {. | |||
There was a problem hiding this comment.
hm, do we need all these directory options? when would the user keep the two separate?
There was a problem hiding this comment.
I'm not sure what you mean. The different names for the validatorsDir option are required, because currently Nim doesn't allow the same field to appear in different branches of the case object.
There was a problem hiding this comment.
I was thinking that we'd collect everything under the data dir and go by convention / relative hardcoding for the rest - one could imagine that symlinks can be used if someone desperately wants to keep these apart.
There was a problem hiding this comment.
I could think of several reasons to keep these separate:
It's customary for system operators to treat passwords in a special way when it comes to back-ups and system provisioning. There are many specialized solutions for this (example). Supporting a layout with separate folders is helpful for this.
It's more arguable whether keystores should be kept separate from the passphrases, but I think this also enables some slightly safer practices such as keeping them both backed-up in separate locations (a malicious actor needs to somehow obtain both).
Another valid reason to separate credentials from the data dir is that tools like MetaMask make it somewhat easy to reuse your credentials on multiple networks. This may become a convenience if the number of testnets increases in the future.
|
right now we have the issue where the passed genesis and the database may mismatch, and we don't handle it well - is this something we want to address in this branch? An option is to separate init from (re)start, where the second step would involve an already created folder structure (with fewer options accepted on the command line) - it's uglier for some reasons (complicates zero-to-hero) but also more easy to reason about in terms of UX - once the two-phase thing works, we'd streamline it again. |
|
Can you clarify what the issue is? Each data dir currently hosts a genesis file and there is some logic trying to check when the information in the data dir needs to be wiped clean. Perhaps your use case is loading a snapshot that is descendant of a previous genesis? In any case, I would suggest address this in a separate PR. |
it holds a genesis file, a database with a genesis and gets a genesis from command line - these are 3 sources of truth which may or may not match. There's a fragile setup in the Better would be to have one source of truth: the database, for example, and remove the genesis file from the data dir - and for example refuse to start if there's a mismatch - the database is valuable - possibly add a To get to one source of truth for the "run" command, the other sources of truth could be covered in an "initialize" command which would simplify the command line options, if nothing else, for a "normal" restart of the application that should "just work" where it last left off. Once that principal use case works well, one can consider extra options that init+run in one step again, but as is, it's very easy to lose your keys, db etc - it's optimized for developers frequently restarting the app on a fresh network, not users accessing the same network over and over. Another possibility is to support multiple genesi in a single data folder, but this feels complicated. |
| ../beacon_chain/spec/[crypto, keystore] | ||
|
|
||
| from strutils import replace | ||
|
|
||
| template `==`*(a, b: ValidatorPrivKey): bool = |
There was a problem hiding this comment.
fwiw, in tests something we've used is to compare the raw bytes representation - this looks like something that needs cleaning up in the blscurve repo as well, don't think that comparison should be there (it's been removed elsewhere). we can leave it for then.
There was a problem hiding this comment.
We usually comparing binary representation and i think == is not present in blscurve.
zah
force-pushed
the
keystores
branch
2 times, most recently
from
71f48ca to
454688c
Compare
beacon_chain/beacon_node.nim
Outdated
| node.addLocalValidators(state) | ||
| for validatorKey in node.config.validatorKeys: | ||
| node.addLocalValidator state, validatorKey | ||
| # Allow some network events to be processed: |
There was a problem hiding this comment.
what are we waiting for here, actually?
There was a problem hiding this comment.
In the testnet simulation, other peers will start connecting to the bootstrap node while it's running this loop (which is suspected to take multiple seconds on the Jenkins instances). I'll go one step further today and will completely separate the attachment of validators from the network initialization steps. Validators can be attached at any point anyway.
Since I'm not able to reproduce the finalization failure locally and it does happen only sporadically, one possible explanation is that the introduction of keystores lead to a slower initialization of the beacon nodes which somehow interferes with their behavior during the initial slots. If increasing the start-up delay fixes the problems, the hypothesis will be confirmed.
…loading of KeyStores
No description provided.