Bitcoin Core hot wallet #210

ben-kaufman · 2020-07-11T17:58:36Z

Closes #58

Adds a new device type: Bitcoin Core. This will create a Bitcoin Core hot wallet from a BIP39 mnemonic as a new signing device, allowing to use it in as any other device (exception is that it is limited to the network type, i.e. the same wallet can't be used both on mainnet and testnet).

To utilize this new device type, go to the new device screen and select Bitcoin Core as the device type. This will open a text field with a proposed, random mnemonic, which you can either use or replace with your own mnemonic. Once created, you can use it just like any other device.

This is still WIP, things left to do:

Encrypted wallet support (either allow only encrypted for security or just do it by default)
UI/UX improvements (open for suggestions).
Foolproof it and proper error handling (e.g. error if invalid mnemonic entered etc.).
Clean up the code a bit.

Edit:
Added optional support for encrypted wallets (an empty passphrase is treated as unencrypted). Also a few improvements to the UI according to @stepansnigirev's comment.

stepansnigirev · 2020-07-12T08:52:15Z

Tested, works!
A few comments:

I would suggest generating a 12-word mnemonic by default, not 24 words. 12 words are unfeasible to brute force as well. Or maybe have a button to regenerate mnemonic with 12 / 24 words.
Do you plan to implement bip39 passwords? It could be just another input field.
UX: We can have two tabs for mnemonic - new and recovery. In the new tab we can display mnemonic in a nice table, in the recovery tab we can keep just a text area.
"Forget the device" button should also delete the hot wallet on the node if possible.

ChristopherA · 2020-07-12T09:42:21Z

You should talk with us (@Fonta1n3 @wolfmcnally and I) about how our standards for exchanging seeds, BIP39, Shamir, and derived keys (soon), and account maps (aka public key only wallet descriptors) for Bitcoin Standup & Fully Noded 2. It would be great if this Bitcoin-Core tool used the same standards.

stepansnigirev · 2020-07-12T12:06:42Z

You should talk with us (@Fonta1n3 @wolfmcnally and I) about how our standards for exchanging seeds, BIP39, Shamir, and derived keys (soon), and account maps (aka public key only wallet descriptors) for Bitcoin Standup & Fully Noded 2. It would be great if this Bitcoin-Core tool used the same standards.

I opened an issue on this topic (#212). Would be nice to support export to Fully Noded and other wallets.

Fonta1n3 · 2020-07-12T13:29:14Z

Closes #58

Adds a new device type: Bitcoin Core. This will create a Bitcoin Core hot wallet from a BIP39 mnemonic as a new signing device, allowing to use it in as any other device (exception is that it is limited to the network type, i.e. the same wallet can't be used both on mainnet and testnet).

To utilize this new device type, go to the new device screen and select Bitcoin Core as the device type. This will open a text field with a proposed, random mnemonic, which you can either use or replace with your own mnemonic. Once created, you can use it just like any other device.

This is still WIP, things left to do:

Use [secrets](https://docs.python.org/3/library/secrets.html) to generate the mnemonic.

Encrypted wallet support (either allow only encrypted for security or just do it by default)

UI/UX improvements (open for suggestions).

Foolproof it and proper error handling (e.g. error if invalid mnemonic entered etc.).

Clean up the code a bit.

Is this a single sig or a signer in a multisig wallet?

Fwiw I added these instructions to easily recover any Specter wallet with Fully Noded here, certainly plan to make it easier by supporting your QR code directly. After importing the descriptors a user can simply add signers to make it spendable in the form of bip39 mnemonics.

stepansnigirev · 2020-07-12T13:46:20Z

Is this a single sig or a signer in a multisig wallet?

It's both, it creates a "device" that can be used as a co-signer in multisig or in a single-sig wallet.
Keys are imported to the Bitcoin Core itself, so they are not stored in Specter.

ben-kaufman · 2020-07-12T13:49:29Z

Is this a single sig or a signer in a multisig wallet?

The way I implemented this is that I separated the signing functionality from the key management. For the key and address management, we just generate the pubkeys from the mnemonic and save it in the same format we do for cold storage devices (like hww). So like now you can add a hardware wallet by importing its pubkeys, here you'll have a similar experience after adding the mnemonic.
For signing, we create a dedicated wallet which manages the private keys, but is not actually used to track balances or anything, but only as a "signing device", i.e. get a PSBT, sign it, and return the result.
This means you can use the hot wallet both as a single-sig and in a multisig construction like you can now with any other device.

Another recent change we made actually made exporting the wallet depend on the device type you use (whatever it supports importing a wallet or not, and its specific format), which means the exporting QRcode you wrote about will not appear in the wallet settings page by default. I didn't have in mind the case of importing the wallet into another wallet software like FN, but I think it is actually an important one. So it will be very good to add a standard exporting method, which will be available in all wallets, regardless of their devices. Adding that will also mean we can export it exactly as FN would expect it, so the user won't need to do the modifications like described in the doc now. Let's continue talking about this in #212 or a new dedicated issue for that.

…esktop into hot-wallet

stepansnigirev

Works great! A few comments (aside from the review):

Add more keys button doesn't work for Bitcoin Core - makes sense to remove it from the device screen if it's a hot wallet.
Display address on device doesn't work for hot storage as well, probably we should show there only hwi devices. But I think it's bug from my HWI refactoring :)

stepansnigirev · 2020-07-13T15:01:34Z

src/cryptoadvance/specter/templates/device/new_device.jinja

+				<p class="note" style="border-radius: 5px; padding:20px; background: rgba(0,0,0,0.1); color: #fff;">
+				<b style="">⚠️ A note on hot wallets:</b><br>
+				This will create a <a style="color: #fff;" href="https://en.bitcoin.it/wiki/Hot_wallet" target="_blank">hot wallet</a> on your Bitcoin Core node.<br>
+				Hot wallets are considered less secure and are not recommended for use with significant amounts. Use with caution and beware the potential risks.
+				</p>


Let's move it to the top, right after device type. Otherwise people will not read this warning.

stepansnigirev · 2020-07-13T15:07:16Z

src/cryptoadvance/specter/controller.py

+                err = "Device name must not be empty"
+            elif device_name in app.specter.device_manager.devices_names:
+                err = "Device with this name already exists"
+            if len(request.form['mnemonic'].split(' ')) not in [12, 15, 18, 21, 24]:


mnemonic module has also a check method:

mnemo = mnemonic.Mnemonic('english') mnemo.check('garlic advice under avocado scene sadness pumpkin skull old mixture drama satoshi') >> False

stepansnigirev · 2020-07-13T15:09:28Z

src/cryptoadvance/specter/devices/bitcoin_core.py

+        # Nested Segwit
+        xpub = bip32.get_xpub_from_path('m/49h/0h/0h')
+        ypub = convert_xpub_prefix(xpub, b'\x04\x9d\x7c\xb2')
+        xpubs += "[%s/49'/0'/0']%s\n" % (master_fpr, ypub)
+        # native Segwit
+        xpub = bip32.get_xpub_from_path('m/84h/0h/0h')
+        zpub = convert_xpub_prefix(xpub, b'\x04\xb2\x47\x46')
+        xpubs += "[%s/84'/0'/0']%s\n" % (master_fpr, zpub)
+        # Multisig nested Segwit
+        xpub = bip32.get_xpub_from_path('m/48h/0h/0h/1h')
+        Ypub = convert_xpub_prefix(xpub, b'\x02\x95\xb4\x3f')
+        xpubs += "[%s/48'/0'/0'/1']%s\n" % (master_fpr, Ypub)
+        # Multisig native Segwit
+        xpub = bip32.get_xpub_from_path('m/48h/0h/0h/2h')
+        Zpub = convert_xpub_prefix(xpub, b'\x02\xaa\x7e\xd3')
+        xpubs += "[%s/48'/0'/0'/2']%s\n" % (master_fpr, Zpub)
+        # Testnet nested Segwit
+        xpub = bip32.get_xpub_from_path('m/49h/1h/0h')
+        upub = convert_xpub_prefix(xpub, b'\x04\x4a\x52\x62')
+        xpubs += "[%s/49'/1'/0']%s\n" % (master_fpr, upub)
+        # Testnet native Segwit
+        xpub = bip32.get_xpub_from_path('m/84h/1h/0h')
+        vpub = convert_xpub_prefix(xpub, b'\x04\x5f\x1c\xf6')
+        xpubs += "[%s/84'/1'/0']%s\n" % (master_fpr, vpub)
+        # Testnet multisig nested Segwit
+        xpub = bip32.get_xpub_from_path('m/48h/1h/0h/1h')
+        Upub = convert_xpub_prefix(xpub, b'\x02\x42\x89\xef')
+        xpubs += "[%s/48'/1'/0'/1']%s\n" % (master_fpr, Upub)
+        # Testnet multisig native Segwit
+        xpub = bip32.get_xpub_from_path('m/48h/1h/0h/2h')
+        Vpub = convert_xpub_prefix(xpub, b'\x02\x57\x54\x83')
+        xpubs += "[%s/48'/1'/0'/2']%s\n" % (master_fpr, Vpub)
+        keys, failed = Key.parse_xpubs(xpubs)


Makes sense to export only xpubs for the current network (only testnet or mainnet) as others will not be available anyways.

stepansnigirev · 2020-07-13T15:10:47Z

src/cryptoadvance/specter/templates/device/components/device_type.jinja

@@ -3,6 +3,7 @@
    <fieldset style="border:none; display: inline;"> 
        <select name="device_type" id="device_type">
            <option value="other">Other</option>
+            <option value="bitcoincore">Bitcoin Core</option>


Let's call it Bitcoin Core (hot wallet)

stepansnigirev · 2020-07-13T15:25:59Z

src/cryptoadvance/specter/templates/device/new_device.jinja

+				<label>Passphrase: </label>
+				<input type="password" name="passphrase" class="inline" placeholder="Wallet passphrase">


To avoid confusion we should call it "Encryption password" as it's not a bip39 password (unlike passwords in hardware wallet).

ben-kaufman added 2 commits July 11, 2020 20:46

MVP - bitcoin core hot wallet

2d477b1

Merge branch 'master' into hot-wallet

9bf8e1a

Encrypted wallets support

fc50323

ben-kaufman added 8 commits July 12, 2020 17:56

Mnemonic improvements

773c936

Fix unencrypted wallet signing issue

3a2e727

Merge branch 'master' into hot-wallet

a3a77c0

Delete wallet file on forget device

b0a0b7e

Merge branch 'hot-wallet' of https://github.com/ben-kaufman/specter-d…

7e99f7d

…esktop into hot-wallet

Cleanup, some error handling, and UI updates

b7773cf

Merge remote-tracking branch 'upstream/master' into hot-wallet

d67584b

Merge branch 'master' into hot-wallet

92ea430

ben-kaufman marked this pull request as ready for review July 13, 2020 14:32

ben-kaufman changed the title ~~MVP - bitcoin core hot wallet~~ Bitcoin Core hot wallet Jul 13, 2020

ben-kaufman requested a review from stepansnigirev July 13, 2020 14:33

ben-kaufman added 2 commits July 13, 2020 17:38

Fix new device ui conflict mistake

f3e59f2

Merge branch 'hot-wallet' of https://github.com/ben-kaufman/specter-d…

f24eca5

…esktop into hot-wallet

stepansnigirev reviewed Jul 13, 2020

View reviewed changes

ben-kaufman added 2 commits July 13, 2020 18:50

Fixes

729d19b

Fix importing xprv

4e0b7ff

stepansnigirev merged commit b2d6509 into cryptoadvance:master Jul 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bitcoin Core hot wallet #210

Bitcoin Core hot wallet #210

ben-kaufman commented Jul 11, 2020 •

edited

Loading

stepansnigirev commented Jul 12, 2020 •

edited

Loading

ChristopherA commented Jul 12, 2020

stepansnigirev commented Jul 12, 2020 •

edited

Loading

Fonta1n3 commented Jul 12, 2020

stepansnigirev commented Jul 12, 2020

ben-kaufman commented Jul 12, 2020 •

edited

Loading

stepansnigirev left a comment

stepansnigirev Jul 13, 2020

stepansnigirev Jul 13, 2020

stepansnigirev Jul 13, 2020

stepansnigirev Jul 13, 2020

stepansnigirev Jul 13, 2020

		<label>Passphrase: </label>
		<input type="password" name="passphrase" class="inline" placeholder="Wallet passphrase">

Bitcoin Core hot wallet #210

Bitcoin Core hot wallet #210

Conversation

ben-kaufman commented Jul 11, 2020 • edited Loading

stepansnigirev commented Jul 12, 2020 • edited Loading

ChristopherA commented Jul 12, 2020

stepansnigirev commented Jul 12, 2020 • edited Loading

Fonta1n3 commented Jul 12, 2020

stepansnigirev commented Jul 12, 2020

ben-kaufman commented Jul 12, 2020 • edited Loading

stepansnigirev left a comment

Choose a reason for hiding this comment

stepansnigirev Jul 13, 2020

Choose a reason for hiding this comment

stepansnigirev Jul 13, 2020

Choose a reason for hiding this comment

stepansnigirev Jul 13, 2020

Choose a reason for hiding this comment

stepansnigirev Jul 13, 2020

Choose a reason for hiding this comment

stepansnigirev Jul 13, 2020

Choose a reason for hiding this comment

ben-kaufman commented Jul 11, 2020 •

edited

Loading

stepansnigirev commented Jul 12, 2020 •

edited

Loading

stepansnigirev commented Jul 12, 2020 •

edited

Loading

ben-kaufman commented Jul 12, 2020 •

edited

Loading