Can this be re-opened, as it still is an issue? It is still not fixed as I have a 24-word phrase that cannot be used in Brave because Brave doesn’t follow the standard HD Wallet algorithm for key derivation from it.

IIUC, #13245 only fixed 12-word phrases, but 24-word phrases are still non-standards compliant. While I understand that this is a huge pain to fix, the pain of Brave mnemonics being standards non-compliant is only going to get worse with additional crypto adoption over time and the longer we wait to start on the road to standards compliance in brave the more painful this will be.

My recommendation on long-term migration strategy:

1. Update the recover UI to have an option for the user to choose between “Externally Created Seed Phrase” and “Brave Created Seed Phrase” (exact wording and UX TBD).
2. For new users start generating standards compliant 24-word seed phrases and at the same time change the recovery UI to prompt for “Externally Created Seed Phrase or Brave Created Seed Phrase after date X” and “Brave Created Seed Phrase before date X” (exact wording and UX TBD).
3. Eventually (years from now) hide the option to recover from a legacy Brave seed phrase somewhere so most users don’t have to see/deal with it but users with ancient seed phrases can still derive.

Another option would be that when the user tries to recover a 24 word phrase, check to see which accounts have assets on the current network so we can helpfully hint to the user which one they probably have.

Okay, this can be the 3rd complaint. I expect most users of Brave Crypto Wallet would not realise the issue exists. If they had known of the issue they probably wouldn’t have choosen the Brave Crypto Wallet in the first place.

Is there a guide for deriving seed words that will restore the crypto wallet addresses to other wallets?

AFAICT we’ve only had 2 complaints of this so far, probably because this bug only occurs if you are going from Brave to another wallet implementation. I agree it would be ideal to be standards-compatible because that’s what users expect, but either way the funds are fully recoverable.

I think the 2 options are:

1. Add lowercase key export option. In the codewords screen, add a disclaimer that restoring wallets from the codewords requires using Brave; for exporting to other wallets, use the private key export. This is my preferred solution since it’s easier.
2. Add lowercase key export option, but also change the wallet derivation process to be standard-compatible; then on wallet restoration in Brave, we ask whether their wallet codewords are v1 or v2 and derive accordingly. This might cause some user confusion if someone doesn’t know whether they are using v1 or v2 codewords, which can also lead to perceived loss of funds.

i did some digging; findings here

how brave does wallet derivation/restoration

1. generate 32 bytes of entropy and runs this through HKDF to generate a “seed” (see step 7 of https://github.com/brave/brave-browser/wiki/Brave-Ethereum-Remote-Client-Wallet-Seed-Information#protocol)
2. calls bip39.entropyToMnemonic to encode it as a mnemonic
3. calls hdkey.fromMasterSeed directly on the 32-byte seed (https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#master-key-generation)
4. upon restoration, we call bip39.mnemonicToEntropy on the mnemonic to convert it back into the seed

how metamask does wallet derivation/restoration

as far as i understand:

1. calls bip39.generateMnemonic() to generate 16 bytes of entropy encoded as a mnemonic. this follows https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki#generating-the-mnemonic.
2. calls mnemonicToSeed, which does some PBKDF2-HMAC-SHA512 on the mnemonic to generate a 64-byte seed: https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki#from-mnemonic-to-seed
3. calls hdkey.fromMasterSeed with the 64-byte seed
4. upon restoration, calls bip39.mnemonicToSeed

unfortunately, i think this will make it difficult for us to encode the brave wallet codewords as other-wallet-compatible code words, because we would need to find the PBKDF2 preimage. if other wallets are willing to take the seed directly as input, that could work.

After a quick skim of the code (way over my head here), is this line correct? https://github.com/brave/brave-core/blob/cd5a08090094fc1a67033e2760c3a067b27e001d/components/brave_sync/crypto/crypto.cc#L148

bool PassphraseToBytes32(const std::string& passphrase,
                         std::vector<uint8_t>* bytes) {
  DCHECK(bytes);
  size_t written;
  bytes->resize(DEFAULT_SEED_SIZE);
  if (bip39_mnemonic_to_bytes(nullptr, passphrase.c_str(), bytes->data(),
                              bytes->size(), &written) != WALLY_OK) {
    LOG(ERROR) << "bip39_mnemonic_to_bytes failed";
    return false;
  }
  return true;
}

bip39_mnemonic_to_bytes only converts the mnemonic into a binary representation, but it does not actually derive the BIP39 seed. That is done by bip39_mnemonic_to_seed, which doesn’t seem to be used anywhere in the code. Maybe someone mistook the result of PassphraseToBytes32 as the actual seed to derive keys with?