Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HD wallet support #64

Closed
ligi opened this issue Jul 17, 2019 · 20 comments
Closed

HD wallet support #64

ligi opened this issue Jul 17, 2019 · 20 comments
Labels
enhancement New feature or request priority:high

Comments

@ligi
Copy link
Member

ligi commented Jul 17, 2019

means one address per payment in via one HD wallet.

reasons:

@ligi ligi added the enhancement New feature or request label Jul 17, 2019
@ligi
Copy link
Member Author

ligi commented Jul 17, 2019

@pipermerriam what does it take to make your HD wallet python implementation production ready? We actually need this to implement this feature

@pipermerriam
Copy link
Collaborator

cc @kclowes

@pipermerriam
Copy link
Collaborator

PR which implemented it:

ethereum/eth-account#33

Branch where it exists:

https://github.com/ethereum/eth-account/tree/feat/hdaccount

Code: https://github.com/ethereum/eth-account/blob/feat/hdaccount/eth_account/hdaccount.py

I'd say to be production ready:

  1. Needs to be tested against any test vectors from the official BIP32 spec
  2. Should be fuzz tested against a known working implementation if possible (maybe just done manually as a sanity check).
  3. Needs to be audited in some way. Most responsible would be a real external security audit. Maybe acceptable middle ground is a thorough review of the code by someone internal to our team against the spec.

And even then, if we were to be using this to receive funds, I'd want to do a bunch of rigorous sanity checking that the addresses and private keys it generates properly match and don't result in unrecoverable funds.

@ligi
Copy link
Member Author

ligi commented Jul 17, 2019

thanks for the input!

1 seems easy - 2 a bit harder but doable 3 even harder but doable

background I am asking: we kind of need it for wave2

@kclowes
Copy link
Collaborator

kclowes commented Jul 18, 2019

@ligi what is the timeline for wave2?

@ligi
Copy link
Member Author

ligi commented Jul 18, 2019

Unfortunately I do not know. Got a very rough estimate of wave1+1month once - but I will try to get a more reliable timeline.
Do you think it would be possible to get this ready in a month?

@kclowes
Copy link
Collaborator

kclowes commented Jul 18, 2019

I think it would be tight, but assuming we can get a security audit in time, I think it could be doable. I think it would be good to have the three of us hop on a call soon and figure out what are the most minimal features we need to get this working.

@ligi
Copy link
Member Author

ligi commented Jul 19, 2019

yes - let's do a call soon.
as far as I see we can/should do things in parallel as none of the 3 points directly touch the code (1+2 is only adding test code) so the audit can start while the tests are written. Or am I missing something her?
Yes it would be nicer to have the audit with 1+2 in place but unfortunately the timeline is tight (again) - so Ideas on how to get this production ready earlier are most welcome. Perhaps we can also call for help online? Could also be a nice answer to all the people that shout online "we would have done better" and tell them this is their chance to let code speak and not just words ..
How much time do you think (1) will take?

This was referenced Jul 19, 2019
@ligi
Copy link
Member Author

ligi commented Jul 19, 2019

closing this issue (for now) as it seems there is rough consensus forming to go with #69 and #70

@pipermerriam
Copy link
Collaborator

@ligi I think we may have come up with an approach that feels both safe and allows use of the HD wallet code.

  1. Use the code from the linked eth-account to pre-generate the addresses for something like the first 1 million order IDs. Run programatic verification that we can indeed access funds at these accounts (verifying that the generated private keys do indeed correlate to the public keys and addresses.
  2. load all of the addresses into a bloom filter
  3. Server can then generate the addresses as they come in, and then use the bloom filter as a sanity check so that we're confident that we are using addresses that we can recover funds from.

I'm not necessarily pushing for this above #69 and #70 but rather posing a possible solution. Let us know if you like that and we can start working on it.

@ligi
Copy link
Member Author

ligi commented Jul 19, 2019

thanks for the input - I tend to lean to #69 and #70 though. Mainly for simplicity reasons. Did you read the comments by @holiman in the chat ?

@ligi
Copy link
Member Author

ligi commented Dec 19, 2019

@ligi
Copy link
Member Author

ligi commented Feb 26, 2020

eth-account should be used @fubuloubu recently PRed HD wallets: ethereum/eth-account#88

@ligi
Copy link
Member Author

ligi commented Feb 26, 2020

Should we isolate the handling of the key from the plugin to improve security?
Idea: the plugin only gets provided with a list of ethereum addresses.
These addresses can be generated from a key on an offline PC.
This PC can then also sign the transactions to get the funds out.

@pipermerriam
Copy link
Collaborator

if we are confident enough in the implementation we should be able to provide the server with only the public part of the key and let it derive the sub-keys. The offline address generation approach is also a totally fine low-tech approach to ensure security and funds availability and might actually be easier to implement so I'm only pointing out the "safe" way to do on-demand address generation on the server to highlight it as an option, not necessarily advocate for it as the right choice.

@fubuloubu
Copy link

As a side note, only private key derivation is currently implemented. It should be fairly easy to support public key derivation too, but that will require a bunch of test cases that I couldn't find. It should be easy enough to create our own test cases however, it's basically a triangular verification of the derived private keys (which we test against the official test vectors) create the given derived public keys (through doing secp256k1 derive). The biggest question in adding this feature is how the API might look like, which is the bigger reason I decided not to implement it in ethereum/eth-account#88

@ligi
Copy link
Member Author

ligi commented Feb 26, 2020

Thanks for the input @pipermerriam and @fubuloubu - how long do you think this change would take?

@fubuloubu
Copy link

I currently don't have a lot of time to implement it, but it wouldn't be too difficult. I could guide someone along if you wanted it sooner rather than later. Not my day job 😆

@ligi
Copy link
Member Author

ligi commented Feb 26, 2020

@fubuloubu thanks for the info! I think we will go with the list then for now ;-)

@kclowes
Copy link
Collaborator

kclowes commented Feb 26, 2020

FWIW I think public key derivation would be straightforward to implement. cc @marcgarreau as he's been working on this stuff too

@ligi ligi closed this as completed May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:high
Projects
None yet
Development

No branches or pull requests

4 participants