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

HDKey derive returns Invalid index #8

Open
Dolu89 opened this issue Oct 11, 2022 · 6 comments
Open

HDKey derive returns Invalid index #8

Dolu89 opened this issue Oct 11, 2022 · 6 comments

Comments

@Dolu89
Copy link

Dolu89 commented Oct 11, 2022

Hi,
I'm implementing LNURL LUD-05 in a react-native mobile app
I have started my code based on this codepen and I adapted it using scure-bip32/39 (version 1.1.0 for both)

Using the same MNEMONIC and LNURL string from the codepen, I got the right derivation path m/138'/2770944631/3393836580/3568300899/2659809102 but the derive method returns me "Invalid index"

Any idea if it can be an issue from scure-bip32? Thanks

Code example

import { HDKey } from '@scure/bip32'
import { HMAC as sha256HMAC } from 'fast-sha256'
import secp256k1 from 'secp256k1'
import * as lnurl from '@zerologin/lnurl'
import { mnemonicToSeed } from "@scure/bip39"

const stringToUint8Array = (str: string) => {
    return Uint8Array.from(str, (x) => x.charCodeAt(0))
}
const seedFromWords = async (mnemonic: string) => {
    const seed = await mnemonicToSeed(mnemonic)
    return Buffer.from(seed)
}

...

const lnurlObject = lnurl.decode('lightning:LNURL1DP68GURN8GHJ7MRFVA58GMNFDENKCMM8D9HZUMRFWEJJ7MR0VA5KU0MTXY7NWCNYXSMKVCEKX3JRSCF4X3SKXWTXXASNGVE5XQ6RZDMXXC6KXDE3VYCRZCENXF3NQVF5XCEXZE3JXVMRGVRY8YURJVNYV43RGDRRVGN8GCT884KX7EMFDCV8DETA')
const lnurlDecoded = lnurlObject.decoded
const k1 = lnurlObject.k1
const domain = lnurlObject.domain


const mn = 'praise you muffin lion enable neck grocery crumble super myself license ghost'
const seed = await seedFromWords(mn)
const root = HDKey.fromMasterSeed(seed)

const hashingKey = root.derive(`m/138'/0`)
const hashingPrivKey = hashingKey.privateKey
console.log({ hashingPrivKey })


if (!hashingPrivKey) throw new Error('Cannot derive pub key')
const derivationMaterial = new sha256HMAC(hashingPrivKey).update(stringToUint8Array(domain)).digest()
console.log({ derivationMaterial })
const pathSuffix = new Uint32Array(derivationMaterial.buffer.slice(0, 16))
console.log({ pathSuffix })
const path = `m/138'/${pathSuffix.join('/')}`
console.log({ path }) // m/138'/2770944631/3393836580/3568300899/2659809102

const linkingKey = root.derive(path) // throw an error "Invalid index"

EDIT: I made some tests using npm lib "bip32" from bitcoinjs and it looks like there is no issue on this lib
Here is a repo to try it: https://github.com/Dolu89/bip32-temp

yarn install
node index.cjs #run bip32
node index.mjs #run @scure/bip32
@paulmillr
Copy link
Owner

You have invalid index, obviously.

The specification clearly says indexes over 2^31-1 are banned. Your numbers are bigger than that.

Bip32 package does not implement spec correctly. They could change their behavior later and your code would stop running.

@junderw
Copy link

junderw commented Oct 11, 2022

From BIP32 spec:

Each extended key has 2^31 normal child keys, and 2^31 hardened child keys. Each of these child keys has an index. The normal child keys use indices 0 through 2^31-1. The hardened child keys use indices 2^31 through 2^32-1. To ease notation for hardened key indices, a number iH represents i+2^31.

Also, the path syntax is only remotely mentioned here, the convention of whether to use m or M for the root, whether m should be used when trying to derive from a non-root state, whether ' or H or h should be used, whether they are optional etc. are noticeably not specified in BIP32.

This is not an issue of spec, but rather, an issue of interpretation of "what is the norm among other implementations?" since the syntax is not specified.


That said, I don't care either way, but changing our interpretation from m/2147483648 and m/0' are parsed to mean the exact same thing. into m/2147483648 throws an error because it is not within 0 <= N <= 2^31-1 is a breaking change, so I would need to see whether a majority of implementations in other languages etc. parse in that way.

If it is the case that m/2147483648 is an Error and not a hardened derive in a large majority of BIP32 implementations across languages, I would be willing to change to match.

@junderw
Copy link

junderw commented Oct 11, 2022

If the spec had said "a string path representation of i+2^31 MUST be represented as iH" then it would be spec... but also it would mean i' (apostrophe style hardened notation) would be non-spec.

There have been instances of BIPs being modified to match convention when ambiguous in the past, so maybe this could be proposed as a clarification modification to BIP32.

@Dolu89 @paulmillr Thanks for bringing this up. Please let me know if either of you want to move forward with proposing a change to BIP32, or if anyone wants to take the time to look up the behavior of other libraries.

@junderw
Copy link

junderw commented Oct 11, 2022

pathSuffix.join('/')

Can also be changed to

Array.from(pathSuffix).map(n => n & 2**31 ? `${n & 2**31-1}'` : `${n}`).join('/')

Which should work with either implementation.

Edit: Also this, which circumvents the string parsing entirely.

[138+2**31].concat(Array.from(pathSuffix)).reduce((hd, idx) => hd.deriveChild(idx), root);

@paulmillr paulmillr reopened this Oct 12, 2022
@paulmillr
Copy link
Owner

paulmillr commented Oct 12, 2022

sigh another disadvantage of bip32 specification. It's too complex: some different solution could achieve similar functionality in a radically simplified fashion.

Adjusting scure's behavior to match other libraries can be beneficial in this case. Keeping the current behavior as-is could also be fine, since it asks users to be precise; also keeping it as-is makes audit results more relevant than if we change it.

@paulmillr
Copy link
Owner

I think the best way to move forward would be to create deriveUnsafe, which would follow the cross-library-compatible behavior.

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

3 participants