Updates FLIP 200 with new proposal updates #727
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/onflow/flow-docs/D3nhtoUALfKt9CT397r8A95X9awj |
| There exist Flow accounts with keys on them which have been generated using the | ||
| Legacy Path. |
There was a problem hiding this comment.
I think you can be explicit here and mention that, up until now, the Ledger application created Flow accounts using a different interpretation of BIP 44 that we now refer to as the legacy path. And to our knowledge, this is the only application that created accounts in this way.
Great idea to mention the legacy path here!
| The prescribed account discovery procedure is as follows: | ||
|
|
||
| 1. Derive the key pair using the legacy path, checking it's use with the | ||
| public key registry. If an address is found, query the Flow network to fetch |
There was a problem hiding this comment.
This is the first mention of "public key registry" in the document, maybe we should define what it is first and assume it is available for the account discovery algorithm
| The legacy path is: | ||
|
|
||
| ``` | ||
| m / 44' / 1' / 769 / 0 / 0 |
There was a problem hiding this comment.
The key derived from a path depends on the curve used. The derivation process gives different raw keys (not talking about account public keys) when used with a different curve. We can see the function as
derive_key(path, curve) (raw_private_key, raw_public_key)
In the case of the legacy path, we will need to precise that the curve used was NIST P-256. Of course, any wallet can try looking for keys from the 2 curves, but I think it's helpful to precise what curve the legacy path has been using.
We might need 2 lines to remind that keys on Flow are ECDSA keys on 2 different curves.
There was a problem hiding this comment.
I agree with Tarak's suggestion. All keys created to date have been on the P-256 curve, so let's specify that alongside the legacy path.
There was a problem hiding this comment.
@tarakby Do we need to be concerned with BLS keys? Or just ECDSA_P256 and ECDSA_secp256k1 ?
There was a problem hiding this comment.
No, BLS is not supported by Flow accounts (BLS is just supported by Cadence, that's why you might see it in Cadence docs).
| public key registry. If an address is found, query the Flow network to fetch | ||
| the account's details. If an account is found, remember the relationship between the | ||
| path used to generate this key, and the account's details. | ||
| 2. Derive a key pair (starting with account index = 0 and key index = |
There was a problem hiding this comment.
In step 2, the algorithm needs to iterate over the supported elliptic curves (2 for now), before incrementing the index. As mentioned above, the raw key itself is different for each curve.
Only if both keys were not found, the algorithm can assume the path is not used.
There was a problem hiding this comment.
Updated to include iterating over both the supported curves, and hashing algorithms as well
| 3.1. If no address is found in the registry | ||
| 3.1.1 If the key index gap limit has been reached without finding any | ||
| addressed in the registry, then go to step 2, incrementing the account index | ||
| by one and starting with key index = 0 again. |
There was a problem hiding this comment.
I think there is no mention of the account index gap yet.
| 3.1. If no address is found in the registry | |
| 3.1.1 If the key index gap limit has been reached without finding any | |
| addressed in the registry, then go to step 2, incrementing the account index | |
| by one and starting with key index = 0 again. | |
| 3.1. If no address is found in the registry | |
| 3.1.1 If the key index gap limit has been reached without finding any | |
| addressed in the registry, then go to step 2, incrementing the account index | |
| by one and starting with key index = 0 again. If the account index gap limit | |
| has been reached, the discovery is over. |
There was a problem hiding this comment.
We introduce the account index gap limit before the discovery algorithm. I want to keep the termination step for the algorithm in step 3, because the terminal state for the algorithm is when both the account index gap limit, and the key index gap limit have been reached
There was a problem hiding this comment.
You're right, the account gap is already defined earlier 👌🏼
What I meant is that the algorithm (steps 1. 2. 3.) does not describe what to do with the account gap (while it describes how to deal with the key gap).
There was a problem hiding this comment.
Ah yes, the account gap limit is increased when the algorithm hits step 3.1.1
| - If adding a key to an existing account: | ||
| - The largest account index that account discovery determines has been | ||
| previously used to generate existing keys set on the account, which has also | ||
| not been used incorrectly to generate keys set on another account. | ||
| - Otherwise, the smallest account index which has not yet been used to | ||
| generate keys set on any account. |
There was a problem hiding this comment.
I think I see what this sentence means 👌🏼 but I'm sure examples will make it clearer 😅 I'm not saying the examples need to be added to this document.
| When generating additional keys for an account, the correct key index to | ||
| use in the path to generate the new key should be the smallest unused key index | ||
| available which has not yet been used to generate a key on the account. |
There was a problem hiding this comment.
This is great, and should be enough.
To avoid confusion we could also add that a path used with at least one curve is considered "used". This would prevent wallets saying "the path has been used with curve_1, but it hasn't been used with key_2 yet, so I'm gonna use it again".
There was a problem hiding this comment.
Added a new conflict resolution section for this purpose 👍
There was a problem hiding this comment.
The new section is great 👌🏼
| <sup>1</sup>Flow supports account deletion, meaning that an address found in the | ||
| registry may refer to a nonexistent account. In this case the account should be | ||
| skipped but discovery should continue. |
There was a problem hiding this comment.
This is great! Should we also add a similar note for revoked keys? I mean we could precise if we consider a path leading to a revoked key, as a "used" path or a "free" path.
There was a problem hiding this comment.
I think it might depend on how the Public Key registry / Hardware Wallet API will have to work. If it returns back addresses for keys that are on the account but are also revoked, i'm less worried about confusion here than if it doesn't return addresses for such keys.
There was a problem hiding this comment.
Yep good point, I think this could be precised on the key registry side.
| BIP 44 defines an implementation of [Bitcoin Improvement Proposal | ||
| 32](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki) (BIP 32) |
There was a problem hiding this comment.
The issue with BIP44 and BIP32 is that they only define the derivation on curve secp256k1.
Referencing BIP32 shouldn't be enough to apply the derivation to another curve, unless we define how we want to generalize the BIP32 technique to other curves.
There is a SLIP that did something similar "SLIP-0010 describes how to derive private and public key pairs for curve types different from secp256k1." but I'm having minor concerns with it.
I'll reach out to Vacuum Labs and maybe Ledger to see if there is a known generalization we can reference, otherwise we might need to describe the generalization ourselves (not long). I'll follow up on this.
There was a problem hiding this comment.
Update: I reached out to Ledger, but I looks like the answer will come after the holidays. Their answer should help us update FLIP200 (the current doc cannot be complete without specifying this curve derivation point unfortunately)
| The legacy path is: | ||
|
|
||
| ``` | ||
| m / 44' / 1' / 769 / 0 / 0 |
There was a problem hiding this comment.
I agree with Tarak's suggestion. All keys created to date have been on the P-256 curve, so let's specify that alongside the legacy path.
* First draft of FLIP: "Application of BIP 44 in Flow Wallets" * Update 20201125-bip-44-multi-account.md * Update FLIP 200 based on discussion feedback * Update account discovery to cover deleted accounts * Update 20201125-bip-44-multi-account.md * Update 20201125-bip-44-multi-account.md * Update prior art * Update flips/20201125-bip-44-multi-account.md Co-authored-by: Rob Myers <rob@robmyers.org> * Updates FLIP 200 with new proposal updates (#727) * Updates FLIP 200 with new proposal updates * Updates FLIP 200 with new proposal updates * Updates FLIP 200 with new proposal updates * Updates FLIP 200 with new proposal updates * Updates FLIP 200 with new proposal updates * Updates FLIP 200 with new proposal updates * Updates FLIP 200 with new proposal updates * Updates FLIP 200 with new proposal updates * Updates FLIP 200 with new proposal updates * Updates FLIP 200 with new proposal updates * Updates FLIP 200 with new proposal updates * Update 20201125-bip-44-multi-account.md * mention SLIP-10 * Specify that Key & Account index gap limits are flexible (#832) * Update 20201125-bip-44-multi-account.md Co-authored-by: Rob Myers <rob@robmyers.org> Co-authored-by: Jeff Doyle <jeffrey.doyle@dapperlabs.com> Co-authored-by: Tarak Ben Youssef <50252200+tarakby@users.noreply.github.com> Co-authored-by: Tarak Ben Youssef <tarak.benyoussef@dapperlabs.com>
Description
Updates FLIP 200 with new proposal updates