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

Why does HierarchicalPath::derivePath() now only derive relative paths? #850

Open
dan-da opened this issue Jun 6, 2020 · 9 comments
Open

Comments

@dan-da
Copy link

dan-da commented Jun 6, 2020

hd-wallet-derive has been using an older version of bitcoin-php. When I just tried to update, path derivation of absolute paths fails with error: "Only relative paths accepted".

See

$parts = $sequences->decodeRelative($path);

I looked for calls within the lib to HierarchicalKeySequence::deriveAbsolute(), but there don't seem to be any.

Please advise...

@afk11
Copy link
Member

afk11 commented Jun 9, 2020

Yea I suppose its a difference to older versions. It didn't make much sense to accept absolute paths for non-root nodes. The old code was too flexible, and I'm pretty sure the old version didn't respect the m / M capitalization effect, so it didn't even do it right.

Non-root keys don't store their path, and you'd never want to do something like:

root = hd(seed)
account = derive(root, "m/44'/0'/0")
address = derive(account, "m/44'/0'/0'/0/0") because it'd derive an unintended key
or
address = derive(account, "m/0/0") because it'd work, but the path is completely meaningless..

Instead the idea is more like this:

root = hd(seed)
account = derive(root, "44'/0'/0'")
address = derive(account, "0/0")

I guess derivePath could be changed to match what people would expect - would the following cover it?

if ($this->depth == 0 && ($path[0] == 'm' || $path[0] == 'M')) {
    list($wasPrivate, $parts) = $sequences->decodeAbsolute($path);
} else {
    $wasPrivate = true;
    $parts = $sequences->decodeRelative($path);
}

and down the bottom

if (!$wasPrivate) {
     $key = $key->withoutPrivate();
}

@dan-da
Copy link
Author

dan-da commented Jun 9, 2020

I think that would be an improvement, yes.

regarding m/M... m is for xprv, and M is for xpub, yes? source

Do any wallets actually enforce that? In my wanderings, I've seen m used for both xprv and xpub most everywhere.

@afk11
Copy link
Member

afk11 commented Jun 10, 2020

regarding m/M... m is for xprv, and M is for xpub, yes? source

yep!

Do any wallets actually enforce that? In my wanderings, I've seen m used for both xprv and xpub most everywhere.

I think that's because both forms refer to the same public key anyway, and the extra bit of info stored in m/M is also available via the extended key's prefix bytes (those ones responsible for xpub/xprv/ypub/zpub).

Whilst importing a wallet from the key, and if the wallet implements BIP44/49/84, the extended key's prefix tells you the entire path (m/44'/0'/0' say) unless the user indicates it's custom somehow. Then the software can save whatever version it likes internally

@dan-da
Copy link
Author

dan-da commented Jun 10, 2020

yeah, so I notice that decodeAbsolute() supports m and M, but does not distinguish between them. should it? eg, disallow M for xprv?

@afk11
Copy link
Member

afk11 commented Jun 10, 2020

I don't really follow the question - it returns an array with two elements - the parts (like decodeRelative) and the value ($parts[0] == 'm'), to tell you which it was. So the caller can recreate the path from this information.

return [
$parts[0] === "m",
$this->decodeDerivation(...array_slice($parts, 1)),
];

@dan-da
Copy link
Author

dan-da commented Jun 10, 2020

ah, so the caller can decide. That seems better. I didn't look at it closely enough.

Are you planning to merge the changes in above comment soon?

@dan-da
Copy link
Author

dan-da commented Sep 8, 2020

bump!

@ynohtna92
Copy link

bump!

#872

@dan-da, it was merged here. Are you able to update hd-wallet-derive with this and using tag releases instead of dev-master, my composer is having issues with grabbing the correct tag for some reason.

@roleenboticario1
Copy link

Hello, can you help me how to send bitcoin from wallet 1 to another wallet? Do you have code for this? Thanks

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

4 participants