Ethereum 2 walletstore

[mcdee]

Initial draft of a standard for storing details of wallets, analogous to keystores.

[axic]

How is a key stored in the non-hd case?

[mcdee]

There is no key for non-deterministic wallets as the keys are created from random data.

[axic]

What is the point supporting this if no key is stored? It seems to be a non very useful feature. I may be just confused by the intent.

[mcdee]

The type of the wallet defines if it is deterministic or not. If deterministic it needs a seed/key, if not it doesn't. So the value is required for some wallets and not others. The standard should allow for both options.

[mcdee]

Is there any progress on this being merged?

If there is confusion about the purpose it may help to look at https://github.com/wealdtech/go-eth2-wallet#usage regarding the purpose of walletstore. Walletstore is a container that defines how keys are generated within a particular wallet, and provides a container for multiple keys (shown as "account"s) in the diagram at that URL).

[axic]

I am really confused of what use the non-deterministic vector has.

The EIP/ERC should be self contained so that its abstraction and motivation clearly explains everything, without the need for looking for external references.

[mcdee]

I've updated the motivation section, which should provide more clarity as to their purpose.

[mcdee]

Did the update help clarify the purpose of this EIP? If not, please let me know what is unclear and I will attempt to provider further detail

[mcdee]

Please could someone let me know what to do to move this along?

[paulhauner]

I'm also not sure if the non-HD wallet carries it's weight. From what I can tell there's nothing that links the non-HD wallet to generated keys/keystores so it might as well not exist.

Maybe you could help me understand by explaining why anyone would want to export a non-HD wallet from one machine to another?

The non-HD wallet option makes the JSON structure non-trivially more complex; the crypto and path keys might or might-not exist based upon the value of type. Unless I'm missing the purpose of it, it would be a shame to say that one can't be compatible with EIP-2386 unless they implement something that has no practical cryptographic purpose.

[mcdee]

Please could this be merged?

[paulhauner]

Thanks for your hard work @mcdee, I am currently implementing this format for Lighthouse.

I think this EIP serves a useful function in allowing users to create n validators which are all backed up by a single encrypted seed. Having it defined as an EIP will assist eth2 implementers standardise and improve interoperability.

I know @protolambda is currently using ethdo and has encouraged Lighthouse to follow suit. I would also be keen to have @CarlBeek's eyes over this, but perhaps he's sick of me nagging him about it and would prefer not to (sorry!).

I am also not convinced that non-HD wallets carry their weight, this is why I have "requested changes". I might be missing the point, though :)

[paulhauner]

I'm also not sure if the non-HD wallet carries it's weight. From what I can tell there's nothing that links the non-HD wallet to generated keys/keystores so it might as well not exist.

Maybe you could help me understand by explaining why anyone would want to export a non-HD wallet from one machine to another?

The non-HD wallet option makes the JSON structure non-trivially more complex; the crypto and path keys might or might-not exist based upon the value of type. Unless I'm missing the purpose of it, it would be a shame to say that one can't be compatible with EIP-2386 unless they implement something that has no practical cryptographic purpose.

[mcdee]

I think there are two questions here: are non-deterministic wallets useful in general, and if so should they be represented in this EIP?

For the first question, I'd say the answer is "yes". A wallet is a logical grouping of accounts, but that doesn't imply those accounts have to be linked in terms of their generation. For example, a large institutional staker may want to have separate keys for each of their clients, but not want those keys to come from the same seed. The logical management ("Customer keys") are separate from the generation process ("hierarchical deterministic from seed" or "non-deterministic"). There are also keys that cannot be generated from a seed, for example the fragment keys that result from a distributed key generation process, and potentially future items such as the keys for vanity addresses (whenever we pick an address format). Again, regardless of their generation mechanism the logical grouping is something that is useful to have (and if we didn't have a non-deterministic walletstore where would these live?).

So on to the second question. I think that if we want non-deterministic wallets to be viable then we should be able to define them the same way as for hierarchical deterministic wallets. The walletstore forms an anchor to both address accounts in the wallet (via UUID for computers or name for users) and provide the logical grouping ("Withdrawal keys", "Validating keys", and of course later down the line transaction wallets of various forms).

Maybe you could help me understand by explaining why anyone would want to export a non-HD wallet from one machine to another?

I would suggest that all of the reasons for exporting a hierarchical deterministic seed would apply to exporting a non-deterministic wallet. Backups (encrypted, obviously), migration (across servers or even just accounts) and cold storage are three that spring to mind.

I'm curious about the "non-trivial" issue with JSON parsing, given I tried to make it easy. The non-deterministic wallet format is a strict subset of the hierarchical deterministic wallet, so could they not share a single schema with the "crypto" and "path" pieces considered optional? Any type-specific logic could branch from the value of the "type" field. What problems are you facing, and is there a change to the EIP that would result in this being easier?

[paulhauner]

are non-deterministic wallets useful in general

I totally agree they're useful.

I'm curious about the "non-trivial" issue with JSON parsing

I don't mean non-trivial in that it's unreasonably difficult to implement, it's just that taking some data structure and declaring that some fields may or may not exist depending on some other value is complexity that results in more LoC and edge-cases to test. It's all achievable, I'm just hesitant to introduce complexity when it comes to cryptography.

is there a change to the EIP that would result in this being easier?

If we assume non-HD wallets are necessary I think you've chosen the ideal way to represent it.

I would suggest that all of the reasons for exporting a hierarchical deterministic seed would apply to exporting a non-deterministic wallet.

I think there are two important reasons missing:

• HD wallets are able to recover previously generated keys.
• HD wallets influence keys generated in the future.

As I understand it, the non-HD wallet has neither of these properties and from a purely cryptographic perspective it serves no purpose.

should [non-HD wallets] be represented in this EIP?

I think this EIP is trying to define two things:

1. A data structure (HD wallets) that, on its own, provides:
   • An encrypted master seed for generating HD keys.
   • A mechanism for maintaining state about which keys have been generated (nextaccount)
2. A data structure (non-HD wallets) that, on its own, is unnecessary but would be useful in some directory-based key management system (DBKMS).

I think both HD and non-HD will be eventually required and I commend your efforts in achieving both in ethdo. I also think this PR does a great job of defining HD; it's minimal and is basically just extending EIP-2335 by adding a nextaccount field.

However, I think that the non-HD wallet is leaking an incomplete version of a DBKMS into this EIP. The only way I can see that a non-HD wallet is useful is if you have some directory structure where there's one wallet and zero-or-many keystores in sub-directories.

I think defining that a DBKMS would be valuable for interoperability. However, there's many things required for a clear definition: the exact directory structure, wallet/keystore filenames, lockfiles, etc.

So, I am left with the following points:

• Both HD wallets and a DBKMS are substantive enough for their own EIPs.
• Non-HD wallets do not serve a purpose unless we assume the existence of a DBKMS.
• The DBKMS is presently undefined.

Given these three points, it is my preference that this EIP stay focused on HD and that non-HD wallets are defined alongside the DBKMS.

This being said, I'm not going to die on this hill and I acknowledge you've put in the hard work to define this. I think I've said my part and I'll leave it here unless you have a convincing counter argument.

[paulhauner]

Changing my review to "comment" as not to impede progress.

[mcdee]

I agree with what you're saying here regarding mixing crypto and structure. I'm thinking that a good way to proceed would be:

• remove the non-deterministic parts from this EIP and focus it on the hierarchical deterministic case
• create a new EIP that provides a structure for the non-deterministic case
• create a new EIP that provides the directory names, locations etc.

How does this sound?

[paulhauner]

How does this sound?

Sounds great to me!

I'm happy to help out with review on those EIPs. I've implemented this wallet and am moving onto the directory structure now, looking to mirror ethdo.

EDIT: I accidentally submitted a half-written comment before I managed to finish it and realise I was wrong. Apologies, please ignore.

[mpetrunic]

I don't think storing seed in wallet store makes sense since you are suppose to generate both signing and withdrawal keys using that seed. If we start to encourage this, users will pass wallet stores to different validator clients which will expose their withdrawal keys. If we go down this route, we should probably change https://github.com/ethereum/EIPs/blob/master/EIPS/eip-2334.md#validator-keys to use two separate seeds for signing and withrawal keys.

Or am I missing something here?

[mcdee]

Totally agree with changing 2334, I never understood why having the same seed for validator and withdrawal keys was any sort of good idea. Their use cases, access requirements and protection methodologies are totally separate.

[paulhauner]

If we start to encourage this, users will pass wallet stores to different validator clients which will expose their withdrawal keys.

To be clear, they only need to use the wallet when they're generating new validators. To run a validator they only need to pass the voting keystore which leaks nothing about the withdrawal key (except perhaps the path that it can be derived with).

[mpetrunic]

To be clear, they only need to use the wallet when they're generating new validators. To run a validator they only need to pass the voting keystore which leaks nothing about the withdrawal key (except perhaps the path that it can be derived with).

Wait, I missing something here. Let's say you wan't to generate and run 20 validators. So you use some local tool to generate this WalletStore with 20 accounts. I imagine you would want to pass this WalletStore to some VC app with password, in which case you would give that app access to seed for withdrawal keys as well.

Or are you suppose to use some local tool to generate 20 keystores with signing keys from WalletStore and pass those to VC app?

[paulhauner]

Or are you suppose to use some local tool to generate 20 keystores with signing keys from WalletStore and pass those to VC app?

This is how we've implemented it and I think it's how ethdo works too.

[mcdee]

I imagine you would want to pass this WalletStore to some VC app with password, in which case you would give that app access to seed for withdrawal keys as well.

Note that there is no benefit to having the withdrawal private key on the validator client. Even in situations where the validator client is generating deposit data, all that is required from the withdrawal side is the public key.

[wemeetagain]

There's a few places that reference keystore as EIP 2333, should be 2335

[rauljordan]

Can we name this as next_account ?

[MicahZoltu]

The Security Considerations section is required before this can be merged as a draft, the other suggested change is just my personal suggestion but not a blocker for merging as draft.

Have you looked into the proposal @Arachnid had a while back for baking the "path" into the mnemonic, so users don't have to keep track of two pieces of information? I don't think it was ever fully pursued, but I generally was a fan.

[MicahZoltu]

@lightclient Can you give feedback on what the CI errors mean?

[lightclient]

@MicahZoltu it looks like it is passing all the required checks: https://travis-ci.org/github/ethereum/EIPs/builds/720190967

The external proofer has been failing for quite a while because there are various external links that don't resolve anymore.

[MicahZoltu]

Ah, sorry about that @lightclient. I kicked CI after pinging you "just in case it was a transient failure" and it appears that it was a transient failure.

[MicahZoltu]

Merging as draft, recommend considering the other comments I and others have left.

[MicahZoltu]

This feels more like motivation than rationale.

Motivation describes why this EIP is generally a good idea.

Rationale describes why specific choices within the EIP were made (e.g., why JSON instead of YML or why constant value 5 instead of 7).

[MicahZoltu]

Consider **MUST** instead of MUST so they stand out a bit more (this is also the dominant style in this repository).

[eschorn1]

Hi,

I was reviewing nim-blscurve and nim-beacon-chain and had some observations on EIP 2386 as a dependency. Note that a companion/copy to this comment is at status-im/nim-blscurve#85

The EIP 2386 specification is silent on a number of aspects including:

• The maximum length of the name string and expectations regarding Unicode normalization.
• The type or maximum allowed nextaccount value.
• The type or maximum expected version value (though this is currently hardcoded and thus not yet necessary).
• Required or expected behavior when extraneous fields are present.

For example, allowing a name string of length one gigabyte is not necessary and may surface downstream impacts stemming from unexpected/unnecessary memory allocations.

While the name string is a simple identifier, different UTF-8 encodings may arise from malicious intent or simply the broad range of participating devices, operating systems, languages and applications. Characters with accents or other modifiers can have multiple correct Unicode encodings. For example, the Á (a-acute) glyph can be encoded as a single character U+00C1 (the "composed" form) or as two separate characters U+0041 then U+0301 (the "decomposed" form). In some cases, the order of a glyph's combining elements is significant and in other cases different orders must be considered equivalent. Normalization is the process of standardizing string representation such that if two strings are canonically equivalent and are normalized to the same normal form, their byte representations will be the same. Only then can string comparison and ordering operations be relied upon. Performing this step is best practice to support user expectations related to rendering consistency.

Regarding nextaccount, values beyond 2**53 are likely not necessary and may encounter problems related to the JavaScript number type having a 53-bit floating-point mantissa. Further, if this value is related to an index in EIP 2333, then a constraint of 2**32 is more reasonable.

Specifying similar constraints for version can be done alongside other modifications for completeness, though this is not currently necessary.

Specifying required or expected behavior when extraneous fields are present will improve implementation interoperability.

Mitigation Recommendation

To summarize, it is recommended to consider a maximum string length for name and to indicate that implementations should immediately normalize this value to the NKFC form per section 2.11.2.B.2 of Unicode Technical Report #36. Additionally, consider specifying the type and maximum values for both the nextaccount and version values, as well as what should happen if extraneous fields are present.

[axic]

@eschorn1 I suggest copying this to the discussion url designated by the EIP: https://ethereum-magicians.org/t/eip-2386-walletstore/3792

[eschorn1]

Ah, I arrived here via that (relatively quiet) link. I'll drop a link/comment there once my new account is unlocked.