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

[Tooling] SLIP-0010 HD Child Key Generation #510

Merged
merged 39 commits into from
Feb 28, 2023
Merged

[Tooling] SLIP-0010 HD Child Key Generation #510

merged 39 commits into from
Feb 28, 2023

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Feb 14, 2023

Description

This PR contains an implementation of SLIP-0010 which allows for deterministic child key generation from a master key. This is done by using the seed of the master key and creating a hmac hash using the sha512 hashing function. The hash is split into 2 []byte slices representing the SecretKey and the ChainCode.

This hmac key can then be used to deterministically generate keys within the BIP-44 path m/44'/635'/%d'. 44 referring to the BIP-44 hardened key purpose of the path and 635' is the coin type for Pocket Network. %d can be any number between 0-2147483647 inclusive. Allowing for a large number of child keys to be deterministically generated and retrieved by index from a single master key's seed.

NOTE All the child keys are ed25519 keys created from the SecretKey of the SLIP HMAC keypair. They are then converted into an encrypted KeyPair interface following the same pattern already found in the keybase.

Some documentation on this process can be seen here

The complete test vectors from the SLIP-0010 spec are included in slip_test.go as well as some pokt specific test cases.

The DeriveChild CLI endpoint has also been added to allow to end users to use the SLIP integration.

Issue

Fixes N/A

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Implement SLIP-0010
  • Integrate child key generation with the keybase
  • Add unit tests for child generation and to cover the SLIP-0010 spec test vectors
  • Add CLI endpoint DeriveChild
  • Update documentation

Testing

  • make develop_test
  • LocalNet w/ all of the steps outlined in the README

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@h5law h5law added tooling tooling to support development, testing et al client work needed to interface with the node (rpc, cli, etc..) labels Feb 14, 2023
@h5law h5law added this to the M1: Pocket PoS (Proof of Stake) milestone Feb 14, 2023
@h5law h5law self-assigned this Feb 14, 2023
@h5law h5law requested a review from Olshansk February 14, 2023 18:54
@h5law h5law changed the title [WIP] SLIP-0010 HD Child Key Generation [WIP][Tooling] SLIP-0010 HD Child Key Generation Feb 14, 2023
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Code is pretty clean 👌

  2. I learnt a bunch of new stuff, so ty

  3. Usually people say to "never roll your own crypto" so what was the motivation here? Were there no go libraries supporting this or was it just for the sake of doing something fun & useful while also learning?

app/client/keybase/README.md Outdated Show resolved Hide resolved
app/client/keybase/keybase.go Outdated Show resolved Hide resolved
app/client/keybase/keybase.go Outdated Show resolved Hide resolved
app/client/keybase/keybase_test.go Outdated Show resolved Hide resolved
app/client/keybase/keybase.go Outdated Show resolved Hide resolved
shared/crypto/slip.go Outdated Show resolved Hide resolved
shared/crypto/slip.go Outdated Show resolved Hide resolved
shared/crypto/slip.go Outdated Show resolved Hide resolved
shared/crypto/slip.go Outdated Show resolved Hide resolved
shared/crypto/slip.go Outdated Show resolved Hide resolved
@h5law
Copy link
Contributor Author

h5law commented Feb 21, 2023

@Olshansk

Usually people say to "never roll your own crypto" so what was the motivation here? Were there no go libraries supporting this or was it just for the sake of doing something fun & useful while also learning?

I had actually found a few libraries - they were all 3-5 years old without any updates. The only recent one is a "demo" implementation by Iota - however because it was called a demo implementation this seemed like it maybe shouldn't be used.

However, after reading through all the different implementations I saw that this is not really any new cryptography but is instead just utilising it in a different manner. I did enjoy doing it myself but my code is very similar to all the other implementations, the only real difference is linking it with our KeyPair interface - most other implementations are using separate key types for theirs but I decided to instead use a separate type and convert it to our existing KeyPair interface as this is simply an abstraction of an ed25519 key anyway.

@h5law h5law requested a review from Olshansk February 21, 2023 15:47
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a deeper dive and everything makes sense now. Appreciate all the added context you provided to help clarify things.

Should be good after this next set of changes

shared/crypto/README.md Outdated Show resolved Hide resolved
shared/crypto/README.md Outdated Show resolved Hide resolved
shared/crypto/README.md Show resolved Hide resolved
shared/crypto/slip.go Outdated Show resolved Hide resolved
shared/crypto/slip.go Outdated Show resolved Hide resolved
app/client/keybase/slip_test.go Outdated Show resolved Hide resolved
app/client/keybase/slip_test.go Outdated Show resolved Hide resolved
app/client/keybase/slip_test.go Outdated Show resolved Hide resolved
app/client/keybase/slip_test.go Outdated Show resolved Hide resolved
shared/crypto/keypair.go Outdated Show resolved Hide resolved
shared/crypto/keypair.go Outdated Show resolved Hide resolved
shared/crypto/README.md Show resolved Hide resolved
path: "m/0'",
seed: seed,
},
name: "TestVector1 Key derivation is deterministic for path `m/0'`",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my personal understanding, since th epath is m/purpose'/coint_type'/child_idx', what is this deriving?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Olshansk So what we actually do to derive the key is first derive a master key m/ then for each segment derive a child - for our use case this means we derive 3 keys before we actually get to the indexed derivation m(key 1)/44'(key 2)/635'(key 3)/%d'(child key index) this is the idea behind the of BIP-44. So we can change the purpose (44') part of the path and with the same seed generate new accounts. For our use case this probably wont be the case but that is what we are actually doing. Many layers to the child 🧅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. So hypothetically (we don't need to do it), an application that wants to maintain privacy by generating lots of different receiving addresses could cache m/44/635 and only update the index, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Olshansk so If someone really wanted they could store the seed of the key generated from m/44'/635' and this is equivalent to the SecretKey of the HMAC key. Then they could derive the indexed child directly from this - however our current setup means this wouldn't work as we force the use of a BIP-44 path when deriving children - this means if they saved m/44'/635' they couldn't use the path 193872' to get that child as this is not valid if they used m/44'/635'/193872' as the path at this point they would not get the right child as they would re-generate the other "layers" of the child key. I hope this makes sense.

It would be possible to change this but I don't think the current use case requires this - maybe in a follow up PR or something

app/client/keybase/slip_test.go Outdated Show resolved Hide resolved
app/client/keybase/keystore.go Outdated Show resolved Hide resolved
app/client/keybase/keybase.go Outdated Show resolved Hide resolved
@h5law h5law requested a review from Olshansk February 24, 2023 15:07
app/client/cli/keys.go Show resolved Hide resolved
app/client/cli/keys.go Outdated Show resolved Hide resolved
@h5law h5law requested a review from Olshansk February 26, 2023 21:23
app/client/keybase/keystore.go Outdated Show resolved Hide resolved
app/client/keybase/keystore.go Outdated Show resolved Hide resolved
app/client/keybase/keystore.go Outdated Show resolved Hide resolved
app/client/keybase/keystore.go Outdated Show resolved Hide resolved
app/client/keybase/keystore.go Outdated Show resolved Hide resolved
shared/crypto/slip/slip.go Show resolved Hide resolved
shared/crypto/slip/slip.go Show resolved Hide resolved
require.NoError(t, err)
}

if err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this if structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this makes sure that if the error is present (for the tests when we want it) it doesn't try to do the rest of the test and throw null pointer exceptions because the other stuff isn't there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a suggestion, should it be a continue then so we run the remainder of the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Olshansk I don't think continue would work as this is not in a for loop. From my understanding the t.Run will run all of the test cases in separate "loops" so the return is only blocking the error cases from failing the tests it shouldn't pass anyway. This return doesn't block the other tests from running

shared/crypto/slip/slip_test.go Show resolved Hide resolved
shared/crypto/slip/slip_test.go Outdated Show resolved Hide resolved
@h5law h5law requested a review from Olshansk February 27, 2023 22:44
}

if err != nil {
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return
continue

require.NoError(t, err)
}

if err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a suggestion, should it be a continue then so we run the remainder of the tests?

app/client/keybase/keystore.go Outdated Show resolved Hide resolved
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL at the last couple of minor comments I left, but otherwise LGTM. Appreciate you bearing through all the back & forth.

Would you be down to present this at a devlog?

@h5law
Copy link
Contributor Author

h5law commented Feb 28, 2023

Would you be down to present this at a devlog?

WiFi issues fixed ✅
New headphones ✅
You will not steal my ⚡ twice 😉

@h5law h5law merged commit feadb65 into main Feb 28, 2023
@h5law h5law deleted the slip-0010-keys branch February 28, 2023 10:36
@h5law h5law restored the slip-0010-keys branch February 28, 2023 10:36
bryanchriswhite added a commit that referenced this pull request Feb 28, 2023
* pokt/main:
  [Tooling] SLIP-0010 HD Child Key Generation (#510)
@Olshansk Olshansk deleted the slip-0010-keys branch February 28, 2023 21:20
@Olshansk
Copy link
Member

WiFi issues fixed ✅ New headphones ✅ You will not steal my ⚡ twice 😉

🤝

Side note: after all the iterations, I think we might have the best golang slip-0010 keygen library out there

bryanchriswhite added a commit that referenced this pull request Mar 1, 2023
* main:
  [Utility] Foundational bugs, tests, code cleanup and improvements (2/3) (#550)
  [CONSENSUS] Find issue with sending metadata request (#548)
  [Tooling] SLIP-0010 HD Child Key Generation (#510)
bryanchriswhite added a commit that referenced this pull request Mar 3, 2023
* main:
  [Libp2p] Add libp2p module directories and helpers (part 1) (#534)
  [P2P, Runtime] Update P2P & base config (part 2) (#535)
  [Utility] Foundational bugs, tests, code cleanup and improvements (2/3) (#550)
  [CONSENSUS] Find issue with sending metadata request (#548)
  [Tooling] SLIP-0010 HD Child Key Generation (#510)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client work needed to interface with the node (rpc, cli, etc..) tooling tooling to support development, testing et al
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants