bitcoin-php icon indicating copy to clipboard operation
bitcoin-php copied to clipboard

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

Open dan-da opened this issue 5 years ago • 9 comments

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 https://github.com/Bit-Wasp/bitcoin-php/blob/b53ce778a30f6dd996f8fa748ed43b2fce3ccb7b/src/Key/Deterministic/HierarchicalKey.php#L324

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

Please advise...

dan-da avatar Jun 06 '20 06:06 dan-da

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();
}

afk11 avatar Jun 09 '20 21:06 afk11

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.

dan-da avatar Jun 09 '20 22:06 dan-da

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

afk11 avatar Jun 10 '20 00:06 afk11

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

dan-da avatar Jun 10 '20 14:06 dan-da

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.

https://github.com/Bit-Wasp/bitcoin-php/blob/b53ce778a30f6dd996f8fa748ed43b2fce3ccb7b/src/Key/Deterministic/HierarchicalKeySequence.php#L34-L37

afk11 avatar Jun 10 '20 15:06 afk11

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 avatar Jun 10 '20 15:06 dan-da

bump!

dan-da avatar Sep 08 '20 17:09 dan-da

bump!

https://github.com/Bit-Wasp/bitcoin-php/pull/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.

ynohtna92 avatar Aug 08 '21 11:08 ynohtna92

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

roleenboticario1 avatar Jul 28 '22 05:07 roleenboticario1