Can create External HD wallet with -externalhd #9728

NicolasDorier · 2017-02-09T09:11:45Z

The user creates a new wallet by running ./bitcoind -externalhd=[ExtPubKey base58].

This make it possible to use methods like getnewaddress, fundrawtransaction and all normal wallet operations on a HD pubkey.

Software built on top of core which need to delegate signing operations to hardware wallet will have almost the same code as if signing was done by Core.

With the introduction of a standard for dealing with hardware wallet signing in the future, I expect that signrawtransaction will just delegate the signing to the hardware wallet.

In this way, there will be no code difference between software using third party solution for signing, and those just using core for signing.

I will use it in my own projects. My HW is giving me the ExtPubKey, and I want to use bitcoin core just for coin selection and tracking. I also did not wanted to break bunch of old code. Ping @jonasschnelli

EDIT: @saleemrashid built HW support for Bitcoin Core on https://github.com/saleemrashid/bitcoin/tree/hardware-wallet based on this PR

jonasschnelli · 2017-02-09T09:15:43Z

src/wallet/walletdb.h

jonasschnelli · 2017-02-09T09:16:36Z

src/wallet/wallet.cpp

s/AddKey/AddWatchOnly/

jonasschnelli · 2017-02-09T09:18:59Z

src/wallet/wallet.cpp

I think returning false in case of isWatchOnly is confusing.
IMO better set the hasSecret boolean with another check of hdChain.isWatchOnly.

I can replace returns bool by void, and make the client responsible to test the validity of the returned Key.

But I fear that a new user would assume that DeriveNewKey always returns a valid CKey.

Yes. I once did a PR where we separate the key/pubkey records: #9298
I guess this would be the cleaner solution... but maybe later.

jonasschnelli · 2017-02-09T09:20:34Z

Concept ACK (will review in detail and test soon).
I think this is a great feature!
Together with #9662 this would allow better HWW/Cold-Storage interaction.

laanwj · 2017-02-09T09:44:10Z

The user create a new wallet by removing the old wallet.dat and running ./bitcoind -hdwatchonly=[ExtPubKey base58].

Instead of removing the wallet, it would also be possible to specify an alternative one with -wallet, I guess?

luke-jr · 2017-02-09T10:11:03Z

How does this work, considering that Core is exclusively hardened derivation right now? Or can it only watch non-Core wallets? O.o

jonasschnelli · 2017-02-09T10:24:46Z

How does this work, considering that Core is exclusively hardened derivation right now? Or can it only watch non-Core wallets? O.o

For the hd-watch-only default keypath, we should probably use Bip44.
This PR makes much more sense if we would have the flexible key path feature #8723

NicolasDorier · 2017-02-09T12:14:21Z

@luke-jr for watchonly hd I am using non hardened derivation. goal is to eventually combine with flexible path.

@laanwj yes. What I mean is that if you specify -hdwatchonly on an already existing wallet, you get an error at startup.

NicolasDorier · 2017-02-09T13:20:01Z

Travis error not related to this PR.

jonasschnelli · 2017-02-09T15:07:33Z

@NicolasDorier: I think in order to pass travis you need to add -hdwatchonly to the check doc script.

NicolasDorier · 2017-02-15T06:50:17Z

I added a commit to track P2PK as well. It would be the same behavior as normal wallet. However I am not sure it is the right approach as P2PK are obsolete. An attacker could disrupt service by sending a P2PK output, when the service does not expect it.

Also, what for P2WPKH ?

NicolasDorier · 2017-02-20T07:12:47Z

I am replicating the behavior of normal wallets, both p2pk and p2pkh are tracked.

The test is failing because I am testing wrong parameter usage, and there is no way in the test framework to assert an exception. Ping @MarcoFalke , same problem as reported by jonas on #9662

maflcko · 2017-02-21T05:01:32Z

qa/rpc-tests/test_framework/util.py

Is this additional optional arg required? I don't think this will ever be used.

Edit: I see you are doing it for consistency, so might be fine...

it is used by assert_start_raises_init_error

Oh, right. I missed the plural s.

NicolasDorier · 2017-02-21T07:08:13Z

I am at loss understanding why there is a permission denied on travis.... I thought I maybe did not had right to /dev/null, and tried to redirect stderr to stdout instead, but same problem. Any idea ?

maflcko · 2017-02-21T13:05:33Z

Any idea

Try

chmod +x qa/rpc-tests/hdwatchonly.py

jonasschnelli · 2017-08-15T19:57:42Z

src/wallet/rpcwallet.cpp

            "  \"unlocked_until\": ttt,           (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n"
            "  \"paytxfee\": x.xxxx,              (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n"
-            "  \"hdmasterkeyid\": \"<hash160>\"     (string) the Hash160 of the HD master pubkey\n"
+            "  \"hdmasterkeyid\": \"<hash160>\"   (string) the Hash160 of the HD master pubkey\n" + 


nit: missing two whitespaces before (string).

jonasschnelli · 2017-08-15T19:58:28Z

src/wallet/wallet.cpp

+        }
+    }
+    else {
+        if (!AddWatchOnly(pubkey, metadata, nCreationTime))


nit: brackets

jonasschnelli · 2017-08-15T20:01:38Z

src/wallet/wallet.cpp

+        do {
+            if (internal) {
+                chainChildKey.Derive(childKey, hdChain.nInternalChainCounter);
+                metadata.hdKeypath = "m/1/" + std::to_string(hdChain.nInternalChainCounter);


for other reviewers: the account key level is dropped here because it's hardened in BIP44. Maybe a source code comment would be good.

jonasschnelli · 2017-08-15T20:07:40Z

Re-Concept ACK. This should really be something we want in 0.16.
I dislike the fact that the extended pubkey needs to be provided via a startup argument, but I guess as long as there is no runtime wallet creation RPC we have to go with that.

jonasschnelli · 2017-08-15T20:07:45Z

Needs rebase.

NicolasDorier · 2017-08-17T02:39:02Z

@jonasschnelli We talked about this PR at Tokyo with @instagibbs and @sipa . Basically we agreed that contrary to what this PR does, the generated keys should not be considered as WATCH_ONLY but as SPENDABLE, even if core do not have the keys, it is still signable.

This mean that we need to refactor the wallet to first decouple a new class: WatchOnlyKeyStore from the CCryptoKeyStore. Then replace CCryptoKeyStore by a CExternalHDKeyStore to handle signing.

I tried to do that in a separate PR, but this goes deep into the rabbit hole, I will need more work on it.

wtogami · 2017-10-30T05:56:36Z

What is the status of this?

NicolasDorier · 2017-10-31T19:34:02Z

So @saleemrashid is using it for hardware support in Bitcoin Core. I am using it in prod for quite a while, so this is stable.

However, this is not the best way to do, ideally we should do like I detailed here #9728 (comment) .
This however requires deeper refactoring which I started on #10980 .

This would need deeper review, and sadly I am out of time these days to follow through and keep rebasing. So if someone wants to take it over, it would probably be better.

instagibbs · 2017-11-06T19:57:23Z

For those interested, I'm rebasing at https://github.com/instagibbs/bitcoin/tree/externalhd2

jonasschnelli · 2018-02-17T10:54:15Z

Rebase required...

NicolasDorier · 2018-02-17T15:02:45Z

@instagibbs is maintaining a rebased version I think do you want to supersede this PR?

instagibbs · 2018-02-17T15:09:08Z

Sure I can open a replacement PR if people are interested. I'm not actively pushing for merge at this time however.

NicolasDorier · 2018-02-17T15:18:45Z

So the reason I put that back on the shelf, is that I think we want to do that after a major overall wallet refactoring that @sipa proposed on https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2#future-design

This PR is creating new addresses as watch-only, this is however not what we want. We want addresses to be MINE, even if we don't have private key. Managing to refactor the code to reach this goal is a can of worms which might be solved by @sipa redesign proposal.

NicolasDorier · 2018-03-06T17:08:59Z

Closing this it seems that we need deeper wallet refactoring as specified by sipa on https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2#future-design

@instagibbs is maintaining https://github.com/instagibbs/bitcoin/tree/externalhd2 for those interested to continue this work.

jonasschnelli reviewed Feb 9, 2017

View reviewed changes

src/wallet/walletdb.h Outdated

Copy link

Contributor

jonasschnelli Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>=?

NicolasDorier reacted with thumbs up emoji

jonasschnelli reviewed Feb 9, 2017

View reviewed changes

src/wallet/wallet.cpp Outdated

Copy link

Contributor

jonasschnelli Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/AddKey/AddWatchOnly/

jonasschnelli reviewed Feb 9, 2017

View reviewed changes

fanquake added the Wallet label Feb 9, 2017

NicolasDorier force-pushed the watchonlyhd branch from ef90ffb to d66e33a Compare February 9, 2017 09:25

NicolasDorier force-pushed the watchonlyhd branch 2 times, most recently from 899e384 to 4d81253 Compare February 9, 2017 12:49

NicolasDorier force-pushed the watchonlyhd branch 3 times, most recently from b79a3ce to 14d9a0d Compare February 15, 2017 13:45

NicolasDorier mentioned this pull request Feb 16, 2017

Add createwallet "disableprivatekeys" option: a sane mode for watchonly-wallets #9662

Merged

NicolasDorier force-pushed the watchonlyhd branch 2 times, most recently from e69b273 to de5b860 Compare February 20, 2017 07:06

NicolasDorier force-pushed the watchonlyhd branch from 8ec7c56 to fc8f811 Compare February 21, 2017 04:20

maflcko reviewed Feb 21, 2017

View reviewed changes

NicolasDorier force-pushed the watchonlyhd branch from fc8f811 to d8c3b37 Compare February 21, 2017 05:06

NicolasDorier force-pushed the watchonlyhd branch 3 times, most recently from e82852f to 2d91dd4 Compare February 23, 2017 03:39

NicolasDorier force-pushed the watchonlyhd branch 2 times, most recently from 61b180b to 1e99dae Compare August 3, 2017 13:20

NicolasDorier changed the title ~~Can create Watch Only HD wallet with -hdwatchonly~~ Can create external HD wallet with -externalhd Aug 3, 2017

NicolasDorier changed the title ~~Can create external HD wallet with -externalhd~~ Can create External HD wallet with -externalhd Aug 3, 2017

NicolasDorier mentioned this pull request Aug 3, 2017

[Refactor] Decouple CKeyStore from CWatchOnlyStore #10980

Closed

NicolasDorier added 4 commits August 3, 2017 14:47

Can create External HD wallet with -externalHD

08f613d

Coins belonging to external HD are safe

bcded2e

[RPC] Add External HD key to getwalletinfo

578e8da

[Tests] ExternalHD tests

1cac7a6

NicolasDorier force-pushed the watchonlyhd branch from 1e99dae to 1cac7a6 Compare August 3, 2017 14:59

jonasschnelli reviewed Aug 15, 2017

View reviewed changes

src/wallet/wallet.cpp

}

}

else {

if (!AddWatchOnly(pubkey, metadata, nCreationTime))

Copy link

Contributor

jonasschnelli Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: brackets

jonasschnelli reviewed Aug 15, 2017

View reviewed changes

laanwj modified the milestones: 0.16.0, 0.17.0 Jan 11, 2018

jonasschnelli mentioned this pull request Feb 17, 2018

Possibility to import XPub (master public key, e.g. from trezor or ledger or electrum) and watch addresses in read-only mode #12449

Closed

NicolasDorier closed this Mar 6, 2018

jnewbery mentioned this pull request Apr 20, 2018

[RFC] Long term plan for wallet command-line args #13044

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Can create External HD wallet with -externalhd #9728

Can create External HD wallet with -externalhd #9728

Uh oh!

Conversation

NicolasDorier commented Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Feb 9, 2017

Uh oh!

laanwj commented Feb 9, 2017

Uh oh!

luke-jr commented Feb 9, 2017

Uh oh!

jonasschnelli commented Feb 9, 2017

Uh oh!

NicolasDorier commented Feb 9, 2017

Uh oh!

NicolasDorier commented Feb 9, 2017

Uh oh!

jonasschnelli commented Feb 9, 2017

Uh oh!

NicolasDorier commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Feb 20, 2017

Uh oh!

maflcko Feb 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Feb 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Feb 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 15, 2017

Uh oh!

jonasschnelli commented Aug 15, 2017

Uh oh!

NicolasDorier commented Aug 17, 2017

Uh oh!

wtogami commented Oct 30, 2017

Uh oh!

NicolasDorier commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Nov 6, 2017

Uh oh!

jonasschnelli commented Feb 17, 2018

Uh oh!

NicolasDorier commented Feb 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Feb 17, 2018

Uh oh!

NicolasDorier commented Feb 17, 2018

Uh oh!

NicolasDorier commented Mar 6, 2018

NicolasDorier commented Feb 9, 2017 •

edited

Loading

NicolasDorier Feb 9, 2017 •

edited

Loading

NicolasDorier commented Feb 15, 2017 •

edited

Loading

maflcko Feb 21, 2017 •

edited

Loading

NicolasDorier commented Feb 21, 2017 •

edited

Loading

NicolasDorier commented Oct 31, 2017 •

edited

Loading

NicolasDorier commented Feb 17, 2018 •

edited

Loading