Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can create External HD wallet with -externalhd #9728

Closed
wants to merge 4 commits into from

Conversation

@NicolasDorier
Copy link
Member

commented Feb 9, 2017

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

src/wallet/walletdb.h Outdated
@@ -59,6 +63,10 @@ class CHDChain
READWRITE(this->nVersion);
READWRITE(nExternalChainCounter);
READWRITE(masterKeyID);
if (this->nVersion <= SUPPORT_WATCHONLY_VERSION) {

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Feb 9, 2017

Member

>=?

src/wallet/wallet.cpp Outdated
}
else {
if (!AddWatchOnly(GetScriptForDestination(pubkey.GetID())))
throw std::runtime_error(std::string(__func__) + ": AddKey failed");

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Feb 9, 2017

Member

s/AddKey/AddWatchOnly/

src/wallet/wallet.cpp Outdated
// update the chain model in the database
if (!CWalletDB(strWalletFile).WriteHDChain(hdChain))
throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed");
return false;

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Feb 9, 2017

Member

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

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Feb 9, 2017

Author Member

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.

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Feb 9, 2017

Member

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

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

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.

@fanquake fanquake added the Wallet label Feb 9, 2017

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:watchonlyhd branch Feb 9, 2017

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

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

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2017

@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 NicolasDorier force-pushed the NicolasDorier:watchonlyhd branch 2 times, most recently Feb 9, 2017

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2017

Travis error not related to this PR.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

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

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2017

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 NicolasDorier force-pushed the NicolasDorier:watchonlyhd branch 3 times, most recently Feb 15, 2017

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:watchonlyhd branch 2 times, most recently Feb 20, 2017

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2017

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

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:watchonlyhd branch Feb 21, 2017

qa/rpc-tests/test_framework/util.py Outdated
assert('bitcoind exited' in str(e)) #node must have shutdown


def start_nodes(num_nodes, dirname, extra_args=None, rpchost=None, timewait=None, binary=None, ignorestderr=False):

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 21, 2017

Member

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...

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Feb 21, 2017

Author Member

it is used by assert_start_raises_init_error

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 21, 2017

Member

Oh, right. I missed the plural s.

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:watchonlyhd branch Feb 21, 2017

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2017

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 ?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

Any idea

Try

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

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:watchonlyhd branch 3 times, most recently Feb 22, 2017

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2017

Rebased, will soon open a new PR, as discussed with @instagibbs watchonlyhd should be renamed into externalhd. Reason is that we want the generated addresses to behave exactly as if they were done by a normal wallet which has the keys. Those generated addresses might be able to be signed correclty by a HW module contrary to a normal watchonly address imported by importaddress.

Thus we keys generated by externalhd wallet should not be considered watchonly.

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:watchonlyhd branch 2 times, most recently Aug 3, 2017

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

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

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:watchonlyhd branch to 1cac7a6 Aug 3, 2017

@@ -2510,7 +2510,8 @@ UniValue getwalletinfo(const JSONRPCRequest& request)
" \"keypoolsize_hd_internal\": xxxx, (numeric) how many new keys are pre-generated for internal use (used for change outputs, only appears if the wallet is using this feature, otherwise external keys are used)\n"
" \"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" +

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Aug 15, 2017

Member

nit: missing two whitespaces before (string).

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

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Aug 15, 2017

Member

nit: brackets

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

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Aug 15, 2017

Member

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

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

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

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

Needs rebase.

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2017

What is the status of this?

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2017

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

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

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

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 17, 2018

Rebase required...

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2018

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

@instagibbs

This comment has been minimized.

Copy link
Member

commented Feb 17, 2018

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

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.