Bit-Wasp / bitcoin-php

Bitcoin implementation in PHP
The Unlicense
1.05k stars 418 forks source link

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

Open dan-da opened 4 years ago

dan-da commented 4 years ago

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...

afk11 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

afk11 commented 4 years ago

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

dan-da commented 4 years ago

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 commented 4 years ago

bump!

ynohtna92 commented 3 years ago

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.

roleenboticario1 commented 2 years ago

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