feat(wallet): make mnemonic bits tweakable, default to 128 bit / 12 words

[UdjinM6]

Issue being fixed or feature implemented

Allow generating 12, 18, 24 words mnemonics. Default to 12 words as it's the most popular option/de-facto a standard now imo.

What was done?

Add -mnemonicbits option, add tests

How Has This Been Tested?

run tests, play with wallets on regtest

Breaking Changes

n/a, old wallets should not be affected

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[PastaPastaPasta]

I like the relatively small diff; has bitcoin never implemented something like this? If so we should probably just take their implementation, but I'm guessing that you looked already. Only other thing is why default to 12 words? It does seem slightly debated still what level is for sure fine, and seems to me best to err on the high end, no?

[knst]

noticed couple more things when carefully checked, see comments.

---

@PastaPastaPasta

has bitcoin never implemented something like this?

$ grep -r biology bitcoin/ #one of the words from bip39
<nothing>

I suppose they doesn't have anything similar at least in core. Some application such as android wallet or 3rd party wallets have this feature.

Only other thing is why default to 12 words

dash's android app and 'Bitcoin wallet' both have 12 mnemonic words.

[knst]

Missing executable flag on wallet_mnemonicbits.py: chmod +x wallet_mnemonicbits.py

[knst]

isn't 160 also accepted as a correct value?

    // check number of words
    if (nWordCount != 12 && nWordCount != 18 && nWordCount != 24) {
        return false;
    }

but when it's generated:

    if (strength % 32 || strength < 128 || strength > 256) {

Probably should be updated 32 to 64: otherwise Generate() is happy with 160 bits, 224 bits but CMnemonic::Check later fails with error: mnemonic with 15 words are invalid mnemonic.

[knst]

I tested it with modified script:

--- a/test/functional/wallet_mnemonicbits.py
+++ b/test/functional/wallet_mnemonicbits.py
@@ -28,6 +28,8 @@ class WalletMnemonicbitsTest(BitcoinTestFramework):
         assert_equal(len(self.nodes[0].dumphdinfo()["mnemonic"].split()), 12)  # 12 words by default
 
         self.log.info("Can have multiple wallets with different mnemonic length loaded at the same time")
+        self.restart_node(0, extra_args=self.extra_args[0] + ["-mnemonicbits=160"])
+        self.nodes[0].createwallet("wallet_160")
         self.restart_node(0, extra_args=self.extra_args[0] + ["-mnemonicbits=192"])
         self.nodes[0].createwallet("wallet_192")
         self.restart_node(0, extra_args=self.extra_args[0] + ["-mnemonicbits=256"])
@@ -35,6 +37,7 @@ class WalletMnemonicbitsTest(BitcoinTestFramework):
         self.nodes[0].createwallet("wallet_256", False, True)  # blank
         self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd()
         assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).dumphdinfo()["mnemonic"].split()), 12)  # 12 words by default
+        assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").dumphdinfo()["mnemonic"].split()), 15)              # 15 words

...

test_framework.authproxy.JSONRPCException: SetMnemonic: invalid mnemonic: `noise glow drift unveil intact fruit mind add claim ankle pumpkin cheap come tuna win` (-1)

[UdjinM6]

Good catch! Indeed more values should be allowed https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki#generating-the-mnemonic

[knst]

slightly tested ACK

[PastaPastaPasta]

utACK for squash merge

[ogabrielides]

utACK