refactor: use @bitcoinerlab/secp256k1 and @noble/secp256k1 by mvayngrib · Pull Request #1 · ExodusMovement/hdkey

mvayngrib · 2023-09-20T20:23:00Z

~~todo: verify correctness 😛~~

alexandrius · 2023-12-05T13:19:03Z

All tests are successful.

Test Results

yarn test
yarn run v1.19.1
$ standard && mocha


  hdkey
    + fromMasterSeed
      ✓ should properly derive the chain path: m
      ✓ should properly derive the chain path: m/0'
      ✓ should properly derive the chain path: m/0'/1
      ✓ should properly derive the chain path: m/0'/1/2'
      ✓ should properly derive the chain path: m/0'/1/2'/2
      ✓ should properly derive the chain path: m/0'/1/2'/2/1000000000
      ✓ should properly derive the chain path: m
      ✓ should properly derive the chain path: m/0
      ✓ should properly derive the chain path: m/0/2147483647'
      ✓ should properly derive the chain path: m/0/2147483647'/1
      ✓ should properly derive the chain path: m/0/2147483647'/1/2147483646'
      ✓ should properly derive the chain path: m/0/2147483647'/1/2147483646'/2
      > m toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
      > m/0' toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
      > m/0'/1 toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
      > m/0'/1/2' toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
      > m/0'/1/2'/2 toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
      > m/0'/1/2'/2/1000000000 toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
      > m toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
      > m/0 toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
      > m/0/2147483647' toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
      > m/0/2147483647'/1 toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
      > m/0/2147483647'/1/2147483646' toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
      > m/0/2147483647'/1/2147483646'/2 toJSON() / fromJSON()
        ✓ should return an object read for JSON serialization
    - privateKey
      ✓ should throw an error if incorrect key size
    - publicKey
      ✓ should throw an error if incorrect key size
      ✓ should not throw if key is 33 bytes (compressed)
      ✓ should not throw if key is 65 bytes (not compressed)
    + fromExtendedKey()
      > when private
        ✓ should parse it
      > when public
        ✓ should parse it
        ✓ should parse it without verification
    > when signing
      ✓ should work
    > when deriving public key
      ✓ should work
    > when private key integer is less than 32 bytes
      ✓ should work
    HARDENED_OFFSET
      ✓ should be set
    > when private key has leading zeros
      ✓ will include leading zeros when hashing to derive child
    > when private key is null
      ✓ privateExtendedKey should return null and not throw
     - when the path given to derive contains only the master extended key
      ✓ should return the same hdkey instance
     - when the path given to derive does not begin with master extended key
      ✓ should throw an error
    - after wipePrivateData()
      ✓ should not have private data
      ✓ should have correct data
      ✓ should be able to verify signatures
      ✓ should not throw if called on hdkey without private data
    Deriving a child key does not mutate the internal state
      ✓ should not mutate it when deriving with a private key
      ✓ should not mutate it when deriving without a private key


  45 passing (107ms)

✨  Done in 1.05s.

@mvayngrib Do you think we need to add more test cases?

alexandrius · 2023-12-05T16:25:41Z

The change assumes correctness of @bitcoinerlab/secp256k1, HDKey's API is tested public and private keys. We can add some tests there.

I'd add create separate tests for @exodus/secp256k1 vs @bitcoinerlab/secp256k1.

publicKeyVerify ~> isPoint
publicKeyConvert ~> pointCompress
privateKeyTweakAdd ~> privateAdd
publicKeyTweakAdd ~> pointAddScalar
ecdsaSign ~> sign
ecdsaVerify ~> verify
privateKeyVerify ~> isPrivate
publicKeyCreate ~> pointFromScalar

Will update more later.

cc @andrejborstnik

@RyanZim Can you also chime in here?

RyanZim · 2023-12-06T19:12:25Z

In reviewing the tests here, one thing that sticks out is the publicKey API. It would be great to have actual data correctness checks added here, to verify that the resulting public key is properly compressed/left compressed:

hdkey/test/hdkey.test.js

Lines 61 to 76 in 472ee65

    
             it('should not throw if key is 33 bytes (compressed)', function () { 
        
               var priv = secureRandom.randomBuffer(32) 
        
               var pub = curve.G.multiply(BigInteger.fromBuffer(priv)).getEncoded(true) 
        
               assert.equal(pub.length, 33) 
        
               var hdkey = new HDKey() 
        
               hdkey.publicKey = pub 
        
             }) 
        
             it('should not throw if key is 65 bytes (not compressed)', function () { 
        
               var priv = secureRandom.randomBuffer(32) 
        
               var pub = curve.G.multiply(BigInteger.fromBuffer(priv)).getEncoded(false) 
        
               assert.equal(pub.length, 65) 
        
               var hdkey = new HDKey() 
        
               hdkey.publicKey = pub 
        
             }) 
        
           })

These test additions ideally should be contributed to upstream and backported here.

alexandrius · 2024-01-10T11:43:55Z

Added more fixtures, point compression test.

@mvayngrib Should I mark "Ready for review"?
Also could you update the head?

headfire94

utACK, i rely on unit tests here)

mvayngrib · 2024-01-10T18:04:54Z

Also could you update the head?

@alexandrius is this PR behind the exodus branch?

alexandrius · 2024-01-10T18:05:51Z

@mvayngrib sorry I meant PR description

mvayngrib · 2024-01-10T18:06:28Z

 {
  "name": "@exodus/hdkey",
-  "version": "2.1.0-exodus.0",
+  "version": "2.1.1-exodus.1",


let's not bundle this in with these changes, reverted e1b5ed9

mvayngrib · 2024-01-10T18:08:05Z

      "private": "xprvA2nrNbFZABcdryreWet9Ea4LvTJcGsqrMzxHx98MMrotbir7yrKCEXw7nadnHM8Dq38EGfSh6dqA9QWTyefMLEcBYJUuekgW4BYPJcr9E7j"
+    },
+    {
+      "seed": "69afbf0608755b3480ca7314c145695c64f973c988790eabb165d19809c7991acecfe4518b1456f274e171d496ddd044942278577b6efcdb69a6374d5341ea0a",


@alexandrius what did u use to generate these?

Extracted from:
src/_local_modules/keys/derive.js createKeyAccess

RyanZim

Quick utACK; did not verify correctness.

Can we get these new tests contributed to upstream?

mvayngrib · 2024-01-10T18:31:30Z

I'd add create separate tests for @exodus/secp256k1 vs @bitcoinerlab/secp256k1.

publicKeyVerify ~> isPoint
publicKeyConvert ~> pointCompress
privateKeyTweakAdd ~> privateAdd
publicKeyTweakAdd ~> pointAddScalar
ecdsaSign ~> sign
ecdsaVerify ~> verify
privateKeyVerify ~> isPrivate
publicKeyCreate ~> pointFromScalar

comparing the two libs, they all look correct to me (the "tweak" methods are in-place vs immutable, but the way we use them it doesn't matter). @alexandrius did u still want to add unit tests for these?

alexandrius · 2024-01-10T18:40:55Z

@mvayngrib I don't think we need anything else, it was quick initial thought

alexandrius · 2024-01-10T18:44:24Z

@RyanZim will do

mvayngrib · 2024-01-10T19:06:49Z

@mvayngrib I don't think we need anything else, it was quick initial thought

for something this core/sensitive, i think u had the right idea when u said to add them 😅

alexandrius · 2024-01-11T08:01:40Z

for something this core/sensitive, i think u had the right idea when u said to add them 😅

I can backport tests to previous version. I did on my end was successful

alexandrius · 2024-01-14T13:03:24Z

Should we go ahead with merge and publish?

alexandrius · 2024-01-14T13:21:03Z

@RyanZim BTW created pull request to upstream
cryptocoinjs#60

mvayngrib · 2024-01-15T14:37:57Z

I can backport tests to previous version. I did on my end was successful

sorry, not following. I mean can we add some unit tests for the underlying functions u listed here #1 (comment)

alexandrius · 2024-01-15T15:57:46Z

@mvayngrib I can but I don't think we should do that since we also have this: ExodusMovement/secp256k1-node#1

mvayngrib · 2024-01-15T16:29:46Z

ah, so the unit tests there will make sure those methods match correctly? sgtm

mvayngrib · 2024-01-15T16:31:15Z

@alexandrius sorry, maybe i'm getting confused. With that PR, why do we even need this one?

alexandrius · 2024-01-15T16:34:00Z

@mvayngrib when this PR was created I think we thought only this PR would be enough to match perf. Another point is to incrementally jump on noble

mvayngrib · 2024-01-15T16:47:08Z

could u walk me through the plan, is it this?

finish unit tests in that PR to make us comfortable with this PR
merge + release + use this
sometime later, revert this and use that PR instead?

alexandrius · 2024-01-15T16:57:31Z

@mvayngrib basically yes. Except swap 1st and 2nd steps. We have all tests to check hdkey correctness comparing the methods above do not belong in here

mvayngrib · 2024-01-15T16:58:43Z

as long as we don't ship this to users before we have that unit test suite, i'm okay with it

alexandrius · 2024-01-15T17:04:22Z

@mvayngrib without that PR we won't be able to ship to users anyway.
However I would argue testing those methods do not belong here anyway since we have hdkey correctness checks

mvayngrib added 4 commits September 20, 2023 16:04

refactor: use @bitcoinerlab/secp256k1

d2a5f5a

refactor: public key compression/verification

3add8fc

refactor: signing/verification

a2d2751

refactor: point add

9a552df

mvayngrib changed the base branch from master to exodus September 20, 2023 20:23

mvayngrib marked this pull request as draft September 20, 2023 20:23

mvayngrib added 2 commits September 20, 2023 16:32

refactor: rm noble dep, use pointFromScalar in bitcoinerlab/secp256k1

6ed469d

chore: rm support for node 10, 12

76ba004

alexandrius added 3 commits January 8, 2024 17:20

fix: uglify-js

1f713db

refactor: change url

4eea2a9

feat: more fixtures, point compression

df15d88

alexandrius requested review from RyanZim and andrejborstnik January 10, 2024 11:32

feat: actual data compress checks

6d78b61

alexandrius requested a review from headfire94 January 10, 2024 13:06

headfire94 approved these changes Jan 10, 2024

View reviewed changes

mvayngrib marked this pull request as ready for review January 10, 2024 18:04

chore: undo version changes in package.json

e1b5ed9

mvayngrib commented Jan 10, 2024

View reviewed changes

RyanZim approved these changes Jan 10, 2024

View reviewed changes

alexandrius merged commit a9882d7 into exodus Jan 15, 2024

Uh oh!

Conversation

mvayngrib commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexandrius commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexandrius commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RyanZim commented Dec 6, 2023

Uh oh!

alexandrius commented Jan 10, 2024

Uh oh!

headfire94 left a comment

Choose a reason for hiding this comment

Uh oh!

mvayngrib commented Jan 10, 2024

Uh oh!

alexandrius commented Jan 10, 2024

Uh oh!

mvayngrib Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

mvayngrib Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

alexandrius Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

RyanZim left a comment

Choose a reason for hiding this comment

Uh oh!

mvayngrib commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexandrius commented Jan 10, 2024

Uh oh!

alexandrius commented Jan 10, 2024

Uh oh!

mvayngrib commented Jan 10, 2024

Uh oh!

alexandrius commented Jan 11, 2024

Uh oh!

alexandrius commented Jan 14, 2024

Uh oh!

alexandrius commented Jan 14, 2024

Uh oh!

mvayngrib commented Jan 15, 2024

Uh oh!

alexandrius commented Jan 15, 2024

Uh oh!

mvayngrib commented Jan 15, 2024

Uh oh!

mvayngrib commented Jan 15, 2024

Uh oh!

alexandrius commented Jan 15, 2024

Uh oh!

mvayngrib commented Jan 15, 2024

Uh oh!

alexandrius commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvayngrib commented Jan 15, 2024

Uh oh!

alexandrius commented Jan 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mvayngrib commented Sep 20, 2023 •

edited

Loading

alexandrius commented Dec 5, 2023 •

edited

Loading

alexandrius commented Dec 5, 2023 •

edited

Loading

mvayngrib commented Jan 10, 2024 •

edited

Loading

alexandrius commented Jan 15, 2024 •

edited

Loading