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

HDNodeWallet derivePath not working properly #4551

Closed
Jouzep opened this issue Jan 18, 2024 · 21 comments
Closed

HDNodeWallet derivePath not working properly #4551

Jouzep opened this issue Jan 18, 2024 · 21 comments
Assignees
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6

Comments

@Jouzep
Copy link

Jouzep commented Jan 18, 2024

Ethers Version

^6.9.2

Search Terms

HDNodeWallet, DerivePath

Describe the Problem

I am trying to derivePath from an HDNodeWallet, but the provided path is not the same path when the wallet is generated
for example i am trying to generate a wallet using Mnemonic and a path
input path="m/44'/60'/0'/0/1"
output path="m/44'/60'/0'/0/1/44'/60'/0'/0/1"

Maybe i have a bad understanding of HDWallet but in my mind they should be the same path

Code Snippet

const ethers = require("ethers");
const path = "m/44'/60'/0'/0/1";

const phrase = "word word word word word word word word word word word word";

const mnemonic = ethers.Mnemonic.fromPhrase(phrase);
const wallet = ethers.HDNodeWallet.fromMnemonic(mnemonic, path);

console.log(wallet.path);
// output: m/44'/60'/0'/0/1
const wallet1 = wallet.derivePath(path);
console.log(wallet1.path);
// output: m/44'/60'/0'/0/1/44'/60'/0'/0/1

Contract ABI

No response

Errors

No response

Environment

node.js (v12 or newer)

Environment (Other)

No response

@Jouzep Jouzep added investigate Under investigation and may be a bug. v6 Issues regarding v6 labels Jan 18, 2024
@niZmosis
Copy link

niZmosis commented Feb 3, 2024

Appears the deviation paths have been different since 6.0.0. We may have to revert to 5.7.2 if you want to derive accounts with the BIP44. As you can see from these tests, the base account from wallet is correct, but if you derive an account they are not correct, and the base account doesn't match derived account 0.

Test with 5.7.2:
`const { ethers } = require("ethers")

function test() {
const seedPhrase = 'escape joke bright reform stem industry cool announce hurt survey blossom wrap'

const wallet = ethers.Wallet.fromMnemonic(seedPhrase)

const path1 = m/44'/60'/0'/0/${0}
const derivedWallet1 = ethers.Wallet.fromMnemonic(seedPhrase, path1)
const path2 = m/44'/60'/0'/0/${1}
const derivedWallet2 = ethers.Wallet.fromMnemonic(seedPhrase, path2)

const metaMaskAccount1 = '0x904c2b17cdf69198eed7004E6b4C99e4C1DdB930'
const metaMaskAccount2 = '0x8Cdd9312b3D5Aa52d9ADccE816F9cfB90363A76b'

console.log('Base Wallet Address:', wallet.address)
console.log('Metamask 1:', metaMaskAccount1)
console.log('Derived 1:', derivedWallet1.address)
console.log('Metamask 2:', metaMaskAccount2)
console.log('Derived 2:', derivedWallet2.address)
}

test()`

Results:
Base Wallet Address: 0x904c2b17cdf69198eed7004E6b4C99e4C1DdB930 Metamask 1: 0x904c2b17cdf69198eed7004E6b4C99e4C1DdB930 Derived 1: 0x904c2b17cdf69198eed7004E6b4C99e4C1DdB930 Metamask 2: 0x8Cdd9312b3D5Aa52d9ADccE816F9cfB90363A76b Derived 2: 0x8Cdd9312b3D5Aa52d9ADccE816F9cfB90363A76b

Test with >=6.0.0
`const seedPhrase =
'escape joke bright reform stem industry cool announce hurt survey blossom wrap'

const wallet = Wallet.fromPhrase(seedPhrase)
const path1 = m/44'/60'/0'/0/${0}
const derivedWallet1 = wallet.derivePath(path1)
const path2 = m/44'/60'/0'/0/${1}
const derivedWallet2 = wallet.derivePath(path2)

const metaMaskAccount1 = '0x904c2b17cdf69198eed7004E6b4C99e4C1DdB930'
const metaMaskAccount2 = '0x8Cdd9312b3D5Aa52d9ADccE816F9cfB90363A76b'

console.log('Base Wallet Address:', wallet.address)
console.log('Metamask 1:', metaMaskAccount1)
console.log('Derived 1:', derivedWallet1.address)
console.log('Metamask 2:', metaMaskAccount2)
console.log('Derived 2:', derivedWallet2.address)`

Results:
Base Wallet Address: 0x904c2b17cdf69198eed7004E6b4C99e4C1DdB930 Metamask 1: 0x904c2b17cdf69198eed7004E6b4C99e4C1DdB930 Derived 1: 0xD3e7f7496373597bd382f8231B5C267952804058 Metamask 2: 0x8Cdd9312b3D5Aa52d9ADccE816F9cfB90363A76b Derived 2: 0x6eB2Da796aee744087F0DF30aFec8501282dfF22

@niZmosis
Copy link

niZmosis commented Feb 4, 2024

Alright I found out what is causing it, and found a temp work around as well. So when you call derivePath, it will call that wallet instances "deriveChild" function. Once a wallet is instantiated, it will hold on to its deviation path with the class prop called "path". So when you call derivePath(), the class will go call deriveChild for each part of the path. What happens is it appends that class prop "path" when making the deviated path which isn't right and which is why it ends up like "m/44'/60'/0'/0/0/44'/60'/0'/0/1". This is why it works fine when first making the wallet as "path" is an empty string. It goes deeper and not sure the full intention of the code so I will leave it at that, as this is more than adding a line of code to fix. But for a work around, do not use "deviatePath()", use the static functions.

Here is where the problem is in hdwallet.ts

` /**

  • Return the child for %%index%%.
    */
    deriveChild(_index: Numeric): HDNodeWallet {
    const index = getNumber(_index, "index");
    assertArgument(index <= 0xffffffff, "invalid index", "index", index);
// Base path
let path = this.path; // This will already have 'm/44'/60'/0'/0/0' when you call derivePath()
if (path) {
  path += "/" + (index & ~HardenedBit);
  if (index & HardenedBit) { path += "'"; }
}

const { IR, IL } = ser_I(index, this.chainCode, this.publicKey, this.privateKey);
const ki = new SigningKey(toBeHex((toBigInt(IL) + BigInt(this.privateKey)) % N, 32));

return new HDNodeWallet(_guard, ki, this.fingerprint, hexlify(IR),
  path, index, this.depth + 1, this.mnemonic, this.provider);

}`

Workaround:

`const { ethers } = require("ethers")

function test() {
const seedPhrase =
'escape joke bright reform stem industry cool announce hurt survey blossom wrap'

const wallet = ethers.HDNodeWallet.fromPhrase(seedPhrase)
const mnemonic = wallet.mnemonic

const path1 = m/44'/60'/0'/0/${0}
const derivedWallet1 = ethers.HDNodeWallet.fromMnemonic(mnemonic, path1)
// const derivedWallet1 = ethers.HDNodeWallet.fromPhrase(seedPhrase, '', path1)
const path2 = m/44'/60'/0'/0/${1}
const derivedWallet2 = ethers.HDNodeWallet.fromPhrase(seedPhrase, '', path2)

const metaMaskAccount1 = '0x904c2b17cdf69198eed7004E6b4C99e4C1DdB930'
const metaMaskAccount2 = '0x8Cdd9312b3D5Aa52d9ADccE816F9cfB90363A76b'

console.log('Wallet Address:', wallet.address)
console.log('Metamask 1:', metaMaskAccount1)
console.log('Derived 1:', derivedWallet1.address)
console.log('Metamask 2:', metaMaskAccount2)
console.log('Derived 2:', derivedWallet2.address)
}

test()`

Results:

Wallet Address: 0x904c2b17cdf69198eed7004E6b4C99e4C1DdB930 Metamask 1: 0x904c2b17cdf69198eed7004E6b4C99e4C1DdB930 Derived 1: 0x904c2b17cdf69198eed7004E6b4C99e4C1DdB930 Metamask 2: 0x8Cdd9312b3D5Aa52d9ADccE816F9cfB90363A76b Derived 2: 0x8Cdd9312b3D5Aa52d9ADccE816F9cfB90363A76b

@Jouzep
Copy link
Author

Jouzep commented Feb 4, 2024

Hello thank you mate very good explanation

@Jouzep Jouzep closed this as completed Feb 4, 2024
@niZmosis
Copy link

We found a work around but the issue you brought up is still a problem if you can reopen it.

@ricmoo
Copy link
Member

ricmoo commented Feb 13, 2024

I want to keep this open to further investigate. I’ll close this again if it is deemed not an issue.

There are also two functions getAccountPath and getIndexedAccountPath, depending if you want to match Ledger or if you want to match MetaMask. Each wallet will have chosen their own standard, so it will take some testing to figure out which to use. I see the MetaMask path followed more often though, but you can also use something like Ledger Live; when you import a mnemonic, if checks the first few accounts on both the Ledger and the MetaMask paths, and if it finds MetaMask transactions, switched to MetaMask mode and tags them with a little icon.

@ricmoo ricmoo reopened this Feb 13, 2024
@ricmoo
Copy link
Member

ricmoo commented Feb 14, 2024

So, the above code should have thrown an error. I'm adding that now: the "m/" part of the path asserts that the depth of the node is 0, i.e. that you are computing the child from the "master" or root node.

I'm adding a constraint that it ensures this is the case, so the code in the OP would throw.

As an aside, for those that wish to compute a large number of child nodes, this should be about 5 times faster, as it keeps a reference to an intermediate node, so those calculations do not need to be replicated:

const wallet = ethers.HDNodeWallet.fromMnemonic(mnemonic, "m/44'/60'/0'/0");
const wallet1 = wallet.derivePath("0");
console.log(wallet1);
const wallet2 = wallet.derivePath("1");
console.log(wallet2);

// Or in a for loop:
for (let i = 0; i < 10; i++) {
  console.log(wallet.deriveChild(i));
}

@ricmoo
Copy link
Member

ricmoo commented Feb 14, 2024

These changes were published in v6.11.1.

Thanks! :)

@ricmoo ricmoo closed this as completed Feb 14, 2024
@ricmoo ricmoo added bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published. and removed investigate Under investigation and may be a bug. labels Feb 14, 2024
@Sean329
Copy link

Sean329 commented Feb 24, 2024

I think the initial issue for which this ticket was opened is still there with v6.11.1 :

input path="m/44'/60'/0'/0/1"
output path="m/44'/60'/0'/0/1/44'/60'/0'/0/1"

So, is this intentional and not an issue? Otherwise why this ticket is closed?

@ricmoo
Copy link
Member

ricmoo commented Feb 25, 2024

@Sean329 No that is not the intended behaviour.

This problem should be corrected in the above change, which is to throw if an attempt is made to declare the current node is the root (i.e. master) node when it isn’t. Can you provide sample code that demonstrates the issue you are having?

@Sean329
Copy link

Sean329 commented Feb 25, 2024

@Sean329 No that is not the intended behaviour.

This problem should be corrected in the above change, which is to throw if an attempt is made to declare the current node is the root (i.e. master) node when it isn’t. Can you provide sample code that demonstrates the issue you are having?

@ricmoo Sure, I'm using v6.11.1 and running the code below, and plz look at the log:

const ethers = require("ethers");
const path0 = "44'/60'/0'/0/0";

const phrase = "word word word word word word word word word word word word";

const mnemonic = ethers.Mnemonic.fromPhrase(phrase);
const wallet = ethers.HDNodeWallet.fromMnemonic(mnemonic, path0);

console.log(wallet.path);  // output: m/44'/60'/0'/0/0

const path1 = "44'/60'/0'/0/1";
const wallet1 = wallet.derivePath(path1);

console.log(wallet1.path); // output: m/44'/60'/0'/0/0/44'/60'/0'/0/1

I expect the wallet1.path to be "m/44'/60'/0'/0/1" instead of being "m/44'/60'/0'/0/0/44'/60'/0'/0/1". Am I misunderstanding some concepts in here? Thanks.

@martines3000
Copy link

I am also getting this error:
Serialized Error: { code: 'INVALID_ARGUMENT', argument: 'path', value: 'm/44/1236/1/0/0', shortMessage: 'cannot derive root path (i.e. path starting with "m/") for a node at non-zero depth 5' }.

The code worked fine before the update to the latest version.

I am kinda confused now. Should my code not work or has the new update introduced a bug ?
Thank you for your help.

@niZmosis
Copy link

niZmosis commented Mar 5, 2024

@Sean329 No that is not the intended behaviour.
This problem should be corrected in the above change, which is to throw if an attempt is made to declare the current node is the root (i.e. master) node when it isn’t. Can you provide sample code that demonstrates the issue you are having?

@ricmoo Sure, I'm using v6.11.1 and running the code below, and plz look at the log:

const ethers = require("ethers");
const path0 = "44'/60'/0'/0/0";

const phrase = "word word word word word word word word word word word word";

const mnemonic = ethers.Mnemonic.fromPhrase(phrase);
const wallet = ethers.HDNodeWallet.fromMnemonic(mnemonic, path0);

console.log(wallet.path);  // output: m/44'/60'/0'/0/0

const path1 = "44'/60'/0'/0/1";
const wallet1 = wallet.derivePath(path1);

console.log(wallet1.path); // output: m/44'/60'/0'/0/0/44'/60'/0'/0/1

I expect the wallet1.path to be "m/44'/60'/0'/0/1" instead of being "m/44'/60'/0'/0/0/44'/60'/0'/0/1". Am I misunderstanding some concepts in here? Thanks.

@Sean329 @martines3000

Here would be your updated code.

`
const basePath = "44'/60'/0'/0"; // or "m/44'/60'/0'/0"

const phrase = "word word word word word word word word word word word word"
const mnemonic = ethers.Mnemonic.fromPhrase(phrase);
const baseWallet = ethers.HDNodeWallet.fromMnemonic(mnemonic, basePath);

const wallet1 = baseWallet.derivePath('0');
console.log(wallet1.path);

const wallet2 = baseWallet.derivePath('1');
console.log(wallet2.path);
`

Notice our basePath leaves off the index, and then when deriving you use what ever index you'd like. Don't use the baseWallet directly as the path isn't complete. If you do provide a complete path when making the wallet, it will be valid. But if you go to derive a wallet from it, you will run into the path concatenation problem I showed earlier in the thread.

@ricmoo What do you think about renaming derivePath to deriveIndex and then instead of a string we pass it a number?

EDIT: ricmoo pointed out there is the deriveChild which takes in a number.

`
const basePath = "44'/60'/0'/0"; // or "m/44'/60'/0'/0"

const phrase = "word word word word word word word word word word word word"
const mnemonic = ethers.Mnemonic.fromPhrase(phrase);
const baseWallet = ethers.HDNodeWallet.fromMnemonic(mnemonic, basePath);

const walletViaPath = baseWallet.derivePath('0');
console.log(walletViaPath.path);

const walletViaChild = baseWallet.deriveChild(0);
console.log(walletViaChild.path);
`

@martines3000
Copy link

martines3000 commented Mar 5, 2024

Thanks for the fast reply. Yes this indeed solved my issue.

I also double checked that I get the same addresses (as with the old approach) with a simple script and it looks good. 💯

EDIT: I didn't test it correctly. If I try it with the following code, I get a different address.

const baseWalletOld =
  HDNodeWallet.fromMnemonic(mnemonic).derivePath(`m/44/1236/1/0/0`);

const baseWalletNew = HDNodeWallet.fromMnemonic(
  mnemonic,
  `m/44/1236/1/0`
).derivePath('0');

console.log(`Old: ${baseWalletOld.address}`);
console.log(`New: ${baseWalletNew.address}`);

@niZmosis
Copy link

niZmosis commented Mar 5, 2024

Thanks for the fast reply. Yes this indeed solved my issue.

I also double checked that I get the same addresses (as with the old approach) with a simple script and it looks good. 💯

EDIT: I didn't test it correctly. If I try it with the following code, I get a different address.

const baseWalletOld =
  HDNodeWallet.fromMnemonic(mnemonic).derivePath(`m/44/1236/1/0/0`);

const baseWalletNew = HDNodeWallet.fromMnemonic(
  mnemonic,
  `m/44/1236/1/0`
).derivePath('0');

console.log(`Old: ${baseWalletOld.address}`);
console.log(`New: ${baseWalletNew.address}`);

Your baseWalletOld isn't correct. When not providing the path to the fromMnemonic it will use the default path which is "export const defaultPath: string = "m/44'/60'/0'/0/0";" at which point the base path of the wallet object is now to long to be able to use the derivePath. Because of that default, your ".derivePath(m/44/1236/1/0/0)" is now going to concatenate to the defaults. So you must provide the base path like before without the index, then use the derivePath with the index you want, not the full path. Your baseWalletNew is the correct way to use it.

@ricmoo
Copy link
Member

ricmoo commented Mar 5, 2024

@niZmosis It already exists, but is called .deriveChild, which accepts a number. :)

@martines3000 Working previously was a bug. When you begin a path with m/ you are indicating you want to enforce the current not is the master node. Previously it was ignored, which is bad. Much of the time the higher nodes are not actually present in memory (a feature baked into the specification), so simply “jumping” to the master node isn’t possible. I could consider adding a feature in the future that would allow crawling back up to it, if enough state is in memory to reconstruct it, but I’d have to think about that more. To get the old address using the buggy version, you can simply leave the m/ off the second path to descend into the HD structure the same way. You should move those funds though, as they won’t be accessible using standard tools like ledger or MetaMask.

If you need any help with this, let me know. :)

@chiro-hiro
Copy link

chiro-hiro commented Mar 27, 2024

@ricmoo .deriveChild will create a path m/44'/60'/0'/0/0/0 which is incompatible with other wallet. Is it an issue? I'm using 6.11.1

@niZmosis
Copy link

@ricmoo .deriveChild will create a path m/44'/60'/0'/0/0/0 which is incompatible with other wallet. Is it an issue? I'm using 6.11.1

What ever you use for the base path when making the wallet instance, will be used for the derived paths. If you don't specify a path when creating the wallet, it will prefix with m/44'/60'/0'/0/0 which is the path MetaMask uses.

@chiro-hiro
Copy link

@ricmoo .deriveChild will create a path m/44'/60'/0'/0/0/0 which is incompatible with other wallet. Is it an issue? I'm using 6.11.1

What ever you use for the base path when making the wallet instance, will be used for the derived paths. If you don't specify a path when creating the wallet, it will prefix with m/44'/60'/0'/0/0 which is the path MetaMask uses.

@niZmosis But my path got 1 extra /0 is that normal?

@niZmosis
Copy link

niZmosis commented Apr 7, 2024

@ricmoo .deriveChild will create a path m/44'/60'/0'/0/0/0 which is incompatible with other wallet. Is it an issue? I'm using 6.11.1

What ever you use for the base path when making the wallet instance, will be used for the derived paths. If you don't specify a path when creating the wallet, it will prefix with m/44'/60'/0'/0/0 which is the path MetaMask uses.

@niZmosis But my path got 1 extra /0 is that normal?

No, it sounds like the base path of the wallet was set with the full path, so when you derive it is appending to that ("44'/60'/0'/0/0") instead of ("44'/60'/0'/0"). Pretty much the wallet object you make, you won't be using directly if you are planning on deriving wallets from it.

`
function test() {
// Base path is being used as a prefix for all derived paths
const basePath = "44'/60'/0'/0"; // or "m/44'/60'/0'/0"
const phrase = "escape joke bright reform stem industry cool announce hurt survey blossom wrap"
const mnemonic = ethers.Mnemonic.fromPhrase(phrase);

const baseWallet = ethers.HDNodeWallet.fromMnemonic(
mnemonic,
basePath
).deriveChild(0);

console.log(Address: ${baseWallet.address});
}
`

@netzulo
Copy link

netzulo commented Jul 6, 2024

like a charm :D , ty to comment of @niZmosis , for V "ethers": "^6.13.1",

import { HDNodeWallet, Mnemonic, ethers } from 'ethers';

class MyProvider {
  wCreate() {
    const wallet = ethers.Wallet.createRandom();
    const mnemonic = wallet?.mnemonic?.phrase;
    if(!mnemonic) throw new Error('Error creating wallet');
    return {
      mnemonic,
      privateKey: wallet.privateKey,
      address: wallet.address
    };
  }
  wCreateSubwallet(mnemonic: string, derivationPath: string = "44'/60'/0'/0/0") {
    // index nodex obtained from mnemonic
    const mnenomicInstance = Mnemonic.fromPhrase(mnemonic);
    const masterNode = HDNodeWallet.fromMnemonic(mnenomicInstance);
    // Derivation path for subwallet
    const subwalletNode = masterNode.derivePath(derivationPath);
    return {
      mnemonic: mnemonic,
      privateKey: subwalletNode.privateKey,
      address: subwalletNode.address
    };
  }
}

THE TEST for JEST

import { describe, expect, test } from '@jest/globals';
import { MyProvider } from '../src/MyProvider';

describe("Can use 'index' namespace", () => {
  let walletUser = {
    mnemonic: '',
    privateKey: '',
    address: '',
  };
  async function walletsFixture() {
    const MyProvider = new MyProvider();
    return { MyProvider, walletUser };
  }
  test('instance class: MyProvider', () => {
    expect(new MyProvider() instanceof MyProvider).toBe(true);
  });

  test('[method] wCreateSubwallet: Generate subwallet from mnemonic', async () => {
    const { cypherProvider } = await walletsFixture();
    const newWallet = cypherProvider.wCreate();
    console.log(newWallet);
    const subWallet = cypherProvider.wCreateSubwallet(newWallet.mnemonic, 0);
    console.log(subWallet);
    expect(subWallet).toHaveProperty('privateKey');
    expect(subWallet).toHaveProperty('address');
    expect(subWallet).toHaveProperty('mnemonic');
    expect(subWallet.mnemonic).toBe(newWallet.mnemonic);
    const subWalletTwo = cypherProvider.wCreateSubwallet(newWallet.mnemonic, 1);
    console.log(subWalletTwo);
    expect(subWalletTwo).toHaveProperty('privateKey');
    expect(subWalletTwo).toHaveProperty('address');
    expect(subWalletTwo).toHaveProperty('mnemonic');
    expect(subWalletTwo.mnemonic).toBe(newWallet.mnemonic);
});
});

tests logs

> tsc -b

  console.log
    {
      mnemonic: 'nut earth exhibit give cloud seminar fish walk orient law good beyond',
      privateKey: '0x00fd6564412144d871cdc331ae3d26bee8076d7e9ff3d4448dc96e143f400bd8',
      address: '0x95c272a43409E3a6a1fDa8437fba92dcE00868c8'
    }

      at Object.<anonymous> (__tests__/index.test.ts:42:13)

  console.log
    {
      mnemonic: 'nut earth exhibit give cloud seminar fish walk orient law good beyond',
      privateKey: '0x826f34f920c6f98f7a6f8e9180003e09606ed724bc0d40b7803278249fdadd4c',
      address: '0xd15D4ca221434610E140A5d930AC4C852309FddA'
    }

      at Object.<anonymous> (__tests__/index.test.ts:44:13)

  console.log
    {
      mnemonic: 'nut earth exhibit give cloud seminar fish walk orient law good beyond',
      privateKey: '0xfd974d8c5768bf43c448f58593965710ec0ca20c73748bad439c65846530e9c5',
      address: '0x326ce4A35701Baa4fD3b0bc5963c00891DED43bD'
    }

      at Object.<anonymous> (__tests__/index.test.ts:50:13)

 PASS  __tests__/index.test.ts
  Can use 'index' namespace
    ✓ instance class: MyProvider (3 ms)
    ✓ [method] wCreateSubwallet: Generate subwallet from mnemonic (63 ms)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6
Projects
None yet
Development

No branches or pull requests

8 participants
@niZmosis @ricmoo @Sean329 @chiro-hiro @netzulo @martines3000 @Jouzep and others