Improve Unit-e BIP39 implementation

[cmihai]

Describe the bug After importing a BIP39 mnemonic and generating the 512-bit seed byte sequence, the Unit-e code does not save the seed, but instead generates the 256-bit master key from it, and uses that as the new seed to generate all subsequent hierarchic keys. As a result, the addresses returned by getnewaddress do not match the ones returned by other BIP39 tools, for example, this one.

To Reproduce Steps to reproduce the behavior:

1. Go to https://iancoleman.io/bip39/ and generate a new mnemonic.
2. Scroll down the page, enter m/0'/0' in the BIP32 "Derivation path" textbox, and check "Use hardened addresses".
3. Launch Unit-e, and import the mnemonic generated at step 1.
4. Call getnewaddress '' 'legacy' to obtain the address at m/0'/0'/0'.
5. Check that it does not match the address derived by the web tool.

Expected behavior The generated addresses should match.

Additional context Bitcoin Core does not support BIP39, so their seed values are always 256-bit, and are stored as regular CKeys in the wallet database (and our code does the same, at the moment). We will have to change it to allow storing seeds up to 512 bits.

[scravy]

I am not so sure. The addresses which are returned by our wallet start with a 3, which means it is a P2SH address. The addresses returned by the tool start with 1, which means it is a P2PKH address. So they can not be compared.

A safer way would be to get the list of reserve keys listreservekeys and compare against the public/private keys which are also generated by the tool.

having said that, the derivation of the seed in our tool is as described in BIP39:

To create a binary seed from the mnemonic, we use the PBKDF2 function with a mnemonic sentence (in UTF-8 NFKD) used as the password and the string "mnemonic" + passphrase (again in UTF-8 NFKD) used as the salt. The iteration count is set to 2048 and HMAC-SHA512 is used as the pseudo-random function. The length of the derived key is 512 bits (= 64 bytes).

In unit-e:

  int nIterations = 2048;

  std::string sSalt = std::string("mnemonic") + sPassword;

  if (0 != mnemonicKdf((uint8_t *)sWordList.data(), sWordList.size(),
                       (uint8_t *)sSalt.data(), sSalt.size(), nIterations,
                       &vSeed[0])) {
    return error<1>("%s: mnemonicKdf failed.", __func__);
  }

and

static int mnemonicKdf(const uint8_t *password, size_t lenPassword,
                       const uint8_t *salt, size_t lenSalt, size_t nIterations,
                       uint8_t *out) {
  //...

  // output length is always 64bytes, only 1 block

  if (nIterations < 1) {
    return 1;
  }

  uint8_t r[64];

  int one = 0x01000000;
  CHMAC_SHA512 ctx(password, lenPassword);
  CHMAC_SHA512 ctx_state = ctx;
  ctx.Write(salt, lenSalt);
  ctx.Write((uint8_t *)&one, 4);
  ctx.Finalize(r);
  memcpy(out, r, 64);

  for (size_t k = 1; k < nIterations; ++k) {
    ctx = ctx_state;
    ctx.Write(r, 64);
    ctx.Finalize(r);

    for (size_t i = 0; i < 64; ++i) {
      out[i] ^= r[i];
    }
  }

  return 0;
}

(mnemonic.cpp line 434).

BIP39 is a little vague on how to setup an HD wallet from that, as it says:

This seed can be later used to generate deterministic wallets using BIP-0032 or similar methods.

[cmihai]

The addresses which are returned by our wallet start with a 3, which means it is a P2SH address. The addresses returned by the tool start with 1, which means it is a P2PKH address. So they can not be compared.

Sorry, I should have specified: I meant that, if you generate a 'legacy' (P2PKH) address in Unit-e, decode it, and strip the version byte and checksum, it should match the one generated by the tool.

Our seed generation algorithm is correct -- the issue is that we don't use the generated seed as specified by BIP32, but insert an extra step between "Generate a seed..." and "Calculate l".

@Ruteri is of the opinion that, since the extra step is deterministic, sticking to the algorithm is not that critical at the moment, and we can postpone this until milestone 1. WDYS?

[scravy]

We do have a test to check precisely that, so your funds should always be restorable from the mnemonic: https://github.com/dtr-org/unit-e/blob/master/test/functional/wallet_importmasterkey.py

But obviously this should be fixed before making it public, as people expect the node to behave properly and to be able to restore the same keys form the same mnemonics on other wallets too.