accountNumber passed to partialpath where change should be used

[zanepocock]

In xpub-lib/src/derivation.js the accountNumber passed in by the user is being used to derive addresses from the change segment of the derivation path. See here:

https://github.com/swan-bitcoin/xpub-tool/blob/0c2cd9f090665f8eb23c935fed28837488444e03/packages/xpub-lib/src/derivation.js#L97

In practice, this might just be a semantics issue, but for clarity's sake it should probably take change (0 or 1) as an input parameter.

It follows that the returned path when passing in an accountNumber of 1 (e.g. path: "m/84'/0'/1'/0/0") would appear to be incorrect. Really we are returning the address at path: "m/84'/0'/1'/1/0"

[landabaso]

I think the change parameter should probably be validated in addressFromExtPubKey as well.

This change was really necessary. There's something I still don't understand, though. I see you still kept accountNumber as a parameter.

But isn't the accountNumber usually already encoded in the xpub?

4 bytes version | 1 byte depth | 4 bytes fingerprint | 4 bytes child number

The child number usually corresponds to the account number when depth is 3. At least in modern/standard HD devices/soft.

When you export an xpub for an account in a ledger nano (for example) it will set depth to 3 and the child number will be set to the account number (with the most significant bit set to 1 since it's hardened).

I checked your code and realized you don't use the accountNumber for anything except for printing the path.

I'm not sure what is the point of doing that.

Wouldn't it be better to validate that the accountNumber is equal to the childNumber when the depth is 3 for example?

BTW, are you planning to update npm with this change?

[zanepocock]

Thanks for the feedback @landabaso !

I agree with your observation about the account number being redundant and the linked PR intends to treat it as such. I just left it in because, since we return the full path, it might be useful for the sake of documentation? If the user knows they're passing in the xpub for account 3, for example, they might have a reason to want this tool to return path: "m/84'/0'/3'/1/0", even if it doesn't change the derived addresses. But I will try to look into your suggestion over the weekend.

Pending internal review, if the linked PR is merged then yes I would expect we will update the npm package.

[zanepocock]

@landabaso if you feel like taking another look at the PR, I implemented your suggestion to derive the account number from the xpub. It was a great idea, I would be curious for your feedback.

[landabaso]

I took a look at it. Have you considered getting the depth and making sure it is == 3?

function getAccountFromExtPubKey(extPubKey) {
  const rawAccountNumber = getExtPubKeyMetadata(extPubKey).index

  //Something like this->
  assert(getExtPubKeyMetadata(extPubKey).depth === 3)

  if (rawAccountNumber > 2147483647) {
    return rawAccountNumber - 2147483648
  }
  return rawAccountNumber
}

Of course, this will only work with derivations using standard paths. Old electrum paths were not following this. Also, I believe that Bitcoin Core also use different derivation paths. But Trezor, Ledger and other should be fine.