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

Increasing wallet flexibility #81

Closed
jaspervdm opened this issue Nov 24, 2018 · 14 comments
Closed

Increasing wallet flexibility #81

jaspervdm opened this issue Nov 24, 2018 · 14 comments

Comments

@jaspervdm
Copy link
Contributor

In order to be able to do a wallet restore, the wallet hides some information in the bulletproofs of the outputs it creates. The proof generation uses a nonce, which is used to derive some of the secret values in the proof. Using this nonce a bulletproof can be rewound to obtain the hidden information.

In the default implementation the hidden information is the amount and the derivation path of the key. The nonce is calculated as hash(commit, root_secret_key). During a restore, a wallet has to attempt a rewind on each of the UTXOs, using the nonce. If succesful, it obtains the amount and derivation path, which tells us that the output belonged to us and how to derive the secret key from the seed.

However, there are two downsides to using this specific nonce.

  • It does not allow for "child wallets", i.e. multiple wallets that can be derived from the seed, where the parent can see and spend all the child wallet outputs but the child wallets themselves can only see/spend their own outputs. This is because calculating the nonce requires knowledge of root_secret_key, which effectively means that all child wallets have full access to all the funds, even from siblings.
  • Much for the same reason, watch-only wallets are not possible. It is not watch-only if you know the secret key, because it allows you to spend the outputs as well

Here are a couple of proposed fixes for this.

  • Use hash(commit, child_secret_key) as nonce. This would enable child wallets, but not watch-only. Moreover, a restore for the main wallet has to rewind each UTXO N times for N child wallets, which is more effort. And in most cases N might not even be known or remembered, which would force you to grind a very large number of them.
  • Use hash(commit, child_public_key) as nonce. This would make both child and watch-only wallets possible, but the grinding problem still exists.
  • Use hash(commit, root_public_key) or hash(commit, hash(root_secret_key)) as nonce. This would also enable both new wallet types, but now each child wallet has 2 values they need to have and keep private, namely their child_secret_key and the root_public_key. Leaking the latter will result in a loss of privacy. Furthermore the child wallets can "see" outputs of their siblings, which in some usecases might be undesirable.

None of the methods seem like a great solution, so I think we should spend some more time and try to come up with a better one. Of course wallet implementers are free to choose their own method of marking/restoring outputs, but it would be great if we could have a standard definition that does the job well enough that most wallets will just use it, making them compatible with each other.

Suggestions are very much welcome!

@ignopeverell
Copy link

Just to be clear, what's slow in grinding is trying all those rewinds, correct? As opposed to the hashing.

@sesam
Copy link

sesam commented Nov 26, 2018

Encrypted wallet backups could be used to evade grinding, and if backups get lost, grinding is still an option.

Does the second proposal hash(commit, child_public_key) as nonce keep siblings separate?

(for posterity, see also yesterdays discussion: https://gitter.im/grin_community/dev?at=5bf95d9e66213138940f6e8f)

@jaspervdm
Copy link
Contributor Author

Just to be clear, what's slow in grinding is trying all those rewinds, correct? As opposed to the hashing.

I didnt do any benchmarks, so I am not sure, but the main problem is that you have to check each outputs potentially up to 2^32 times..

@ignopeverell
Copy link

What I'm getting at is that if it's the hash operation on many values that's expensive, we can concentrate on that and try to precompute as much as possible. We could possibly make it so that combining each possibility with the commitment is extremely fast (perhaps just a scalar addition). But if hashing is negligible compared to the rewind then there's really no point, and it's the rewind we need to concentrate on.

@garyyu
Copy link
Contributor

garyyu commented Feb 4, 2019

@jaspervdm Could you update the status of these proposals?

And one question here about the sharing of this nonce with watch-only wallet, doesn't the shared nonce also reveal the blinding (i.e. the secret key) to the watch-only wallet?

The bullet proof rewind will get that blinding together with the value and message, iirc.

@jaspervdm
Copy link
Contributor Author

Could you update the status of these proposals?

Status has been unchanged since I opened the issue. The nonce is still hash(commit, root_secret_key). I am thinking however to open a PR that adds a configuration option to the keychain, where you can specify which nonce derivation scheme should be used.

The bullet proof rewind will get that blinding together with the value and message, iirc.

The rewind will reveal the value, message and derivation path. So a watch only wallet won't be able to extract the blinding factor.

@ignopeverell
Copy link

As mentioned during the dev meeting, I think it'd be useful to normalize the different schemes and embed the scheme information somewhere so wallets can check whether they support said scheme or bail loudly right away. Main issue is where to embed that information.

@garyyu
Copy link
Contributor

garyyu commented Mar 5, 2019

Remind this:

The bullet proof rewind will get that blinding together with the value and message, iirc.

The rewind will reveal the value, message and derivation path. So a watch only wallet won't be able to extract the blinding factor.

The rewind will get the blinding also, so we can't share nonce with watch-only wallet.

@DavidBurkett
Copy link
Contributor

Why do we store the blinding factor? Wouldn't it be better to always just store the derivation path?

Also, important note about one of the original proposals:

Use hash(commit, root_public_key) or hash(commit, hash(root_secret_key)) as nonce. This would also enable both new wallet types, but now each child wallet has 2 values they need to have and keep private, namely their child_secret_key and the root_public_key.

hash(root_secret_key) should be fine, but giving out root_public_key to child wallets should never be done. Given a child_secret_key and the root_public_key, I'm pretty sure you can trivially obtain the root_secret_key.

@jaspervdm
Copy link
Contributor Author

Why do we store the blinding factor? Wouldn't it be better to always just store the derivation path?

Currently, both are stored. I think with some small modification to libsecp we could no longer hide the blinding factor but I have to look into it to be sure.

@quentinlesceller
Copy link
Member

Maybe this should issue should be moved to https://github.com/mimblewimble/grin-wallet?

@jaspervdm
Copy link
Contributor Author

Sure, we could transfer it. Don't think I have the permissions to do it though

@0xmichalis
Copy link
Contributor

0xmichalis commented Apr 7, 2019

@jaspervdm what kind of perms do you need? I think by "move" @quentinlesceller meant "close this issue and open a new one in grin-wallet"

EDIT: Apparently Github supports issue transfer now 🍾

@lehnberg
Copy link
Collaborator

lehnberg commented Apr 8, 2019

@Kargakis https://help.github.com/en/articles/transferring-an-issue-to-another-repository

Don't think it makes sense to close and re-open for all the issues that needs moving. Better to try to preserve issue history.

@ignopeverell are you able to move this to /grin-wallet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants