Can create External HD wallet with -externalhd #9728

NicolasDorier commented at 9:11 AM on February 9, 2017: contributor

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

in src/wallet/walletdb.h:None in 4bca1ceca0 outdated

  62 | @@ -59,6 +63,10 @@ class CHDChain
  63 |          READWRITE(this->nVersion);
  64 |          READWRITE(nExternalChainCounter);
  65 |          READWRITE(masterKeyID);
  66 | +        if (this->nVersion <= SUPPORT_WATCHONLY_VERSION) {

jonasschnelli commented at 9:15 AM on February 9, 2017:

>=?

in src/wallet/wallet.cpp:None in 4bca1ceca0 outdated

 131 | +        if (!AddKeyPubKey(secret, pubkey))
 132 | +            throw std::runtime_error(std::string(__func__) + ": AddKey failed");
 133 | +    }
 134 | +    else {
 135 | +        if (!AddWatchOnly(GetScriptForDestination(pubkey.GetID())))
 136 | +            throw std::runtime_error(std::string(__func__) + ": AddKey failed");

jonasschnelli commented at 9:16 AM on February 9, 2017:

s/AddKey/AddWatchOnly/

in src/wallet/wallet.cpp:None in 4bca1ceca0 outdated

 211 | +        } while (HaveWatchOnly(GetScriptForDestination(childKey.pubkey.GetID())));
 212 | +        pubKey = childKey.pubkey;
 213 | +        // update the chain model in the database
 214 | +        if (!CWalletDB(strWalletFile).WriteHDChain(hdChain))
 215 | +            throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed");
 216 | +        return false;

jonasschnelli commented at 9:18 AM on February 9, 2017:

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

NicolasDorier commented at 9:45 AM on February 9, 2017:

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.

jonasschnelli commented at 9:58 AM on February 9, 2017:

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 commented at 9:20 AM on February 9, 2017: contributor

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 added the label Wallet on Feb 9, 2017

NicolasDorier force-pushed on Feb 9, 2017

laanwj commented at 9:44 AM on February 9, 2017: member

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 commented at 10:11 AM on February 9, 2017: member

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

jonasschnelli commented at 10:24 AM on February 9, 2017: contributor

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 https://github.com/bitcoin/bitcoin/pull/8723

NicolasDorier commented at 12:14 PM on February 9, 2017: contributor

@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 force-pushed on Feb 9, 2017

NicolasDorier commented at 1:20 PM on February 9, 2017: contributor

Travis error not related to this PR.

jonasschnelli commented at 3:07 PM on February 9, 2017: contributor

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

NicolasDorier commented at 6:50 AM on February 15, 2017: contributor

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 force-pushed on Feb 15, 2017

NicolasDorier force-pushed on Feb 20, 2017

NicolasDorier commented at 7:12 AM on February 20, 2017: contributor

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 https://github.com/bitcoin/bitcoin/pull/9662

NicolasDorier force-pushed on Feb 21, 2017

in qa/rpc-tests/test_framework/util.py:None in fc8f8111a1 outdated

 367 | +        raise
 368 | +    except Exception as e:
 369 | +        assert('bitcoind exited' in str(e)) #node must have shutdown
 370 | +    
 371 | +
 372 | +def start_nodes(num_nodes, dirname, extra_args=None, rpchost=None, timewait=None, binary=None, ignorestderr=False):

MarcoFalke commented at 5:01 AM on February 21, 2017:

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

NicolasDorier commented at 5:18 AM on February 21, 2017:

it is used by assert_start_raises_init_error

MarcoFalke commented at 1:04 PM on February 21, 2017:

Oh, right. I missed the plural s.

NicolasDorier force-pushed on Feb 21, 2017

NicolasDorier commented at 7:08 AM on February 21, 2017: contributor

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 commented at 1:05 PM on February 21, 2017: member

Any idea

Try

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

NicolasDorier force-pushed on Feb 22, 2017

NicolasDorier force-pushed on Feb 23, 2017

NicolasDorier force-pushed on Mar 6, 2017

NicolasDorier force-pushed on Mar 8, 2017

NicolasDorier force-pushed on Mar 10, 2017

NicolasDorier force-pushed on Mar 13, 2017

NicolasDorier commented at 8:05 AM on March 13, 2017: contributor

rebased, and cleaned up the commits for better review.

saleemrashid commented at 5:49 PM on March 13, 2017: none

As far as I understand, Bitcoin Core uses m/0'/0'/k' whereas BIP 44 uses m/0'/0/k and m/0'/1/k. It makes most sense for this PR to implement m/0/k (and, with #9294, m/1/k). This allows you to use an xpub derived from m/0' (or any other account) by your hardware wallet, for example.

However, this means that you are doing one less derivation than with xprv in Bitcoin Core. I don't think this is too much of an issue since m/0'/0'/k' is in no way compatible with m/0'/0/k anyway.

NicolasDorier commented at 2:05 AM on March 14, 2017: contributor

@saleemrashid very good point. Will make your change once #9294 is merged.

in src/wallet/walletdb.h:55 in d15d07e205 outdated

  51 | +    bool isWatchOnly = false;
  52 |  
  53 | -    static const int CURRENT_VERSION = 1;
  54 | +    static const int FIRST_VERSION = 1;
  55 | +    static const int SUPPORT_WATCHONLY_VERSION = 2;
  56 | +    static const int CURRENT_VERSION = 2;

jonasschnelli commented at 3:40 PM on March 17, 2017:

nit: CURRENT_VERSION = SUPPORT_WATCHONLY_VERSION.

in src/wallet/wallet.cpp:167 in d15d07e205 outdated

 176 | +        if (!CWalletDB(strWalletFile).WriteHDChain(hdChain))
 177 | +            throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed");
 178 | +    }
 179 | +    else if (IsHDWatchOnly()) {
 180 | +        CExtPubKey& masterKey = hdChain.masterPubKey;             //hd master key
 181 | +        CExtPubKey accountKey;            //key at m/0

jonasschnelli commented at 3:50 PM on March 17, 2017:

To make this compatible with BIP44, can we not just use m as the BIP44 account key. No hardened derivation would then be involved and users could use their BIP44 xpub's to generate watchonlys. The externalChainChildKey would then be m/0 (instead of m/0/0)

saleemrashid commented at 5:14 PM on March 17, 2017:

@jonasschnelli #9728 (comment)

jonasschnelli commented at 7:31 AM on March 20, 2017:

Ah.. Wasn't aware that @saleemrashid already pointed this out. Great.

in src/wallet/wallet.cpp:172 in d15d07e205 outdated

 181 | +        CExtPubKey accountKey;            //key at m/0
 182 | +        CExtPubKey externalChainChildKey; //key at m/0/0
 183 | +        CExtPubKey childKey;              //key at m/0/0/<n>
 184 | +
 185 | +        // derive m/0
 186 | +        masterKey.Derive(accountKey, BIP32_NONHARDENED_KEY_LIMIT);

jonasschnelli commented at 3:51 PM on March 17, 2017:

IMO this constant (BIP32_NONHARDENED_KEY_LIMIT) is superfluous. Just use 0.

in src/wallet/wallet.cpp:179 in d15d07e205 outdated

 188 | +        // derive m/0/0
 189 | +        accountKey.Derive(externalChainChildKey, BIP32_NONHARDENED_KEY_LIMIT);
 190 | +
 191 | +        // derive child key at next index, skip keys already known to the wallet
 192 | +        do {
 193 | +            externalChainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_NONHARDENED_KEY_LIMIT);

jonasschnelli commented at 3:51 PM on March 17, 2017:

Also here, ... just use hdChain.nExternalChainCounter instead of hdChain.nExternalChainCounter | BIP32_NONHARDENED_KEY_LIMIT (no need to bitwise OR it with 0).

in src/wallet/wallet.cpp:114 in d15d07e205 outdated

 112 |          SetMinVersion(FEATURE_COMPRPUBKEY);
 113 |  
 114 | -    CPubKey pubkey = secret.GetPubKey();
 115 | -    assert(secret.VerifyPubKey(pubkey));
 116 | +    if (!IsHDWatchOnly())
 117 | +        assert(secret.VerifyPubKey(pubkey));

jonasschnelli commented at 3:57 PM on March 17, 2017:

I think there is no way to verify the correctness of the derived pubkey?

in src/wallet/walletdb.h:89 in d15d07e205 outdated

  62 | @@ -59,6 +63,11 @@ class CHDChain
  63 |          READWRITE(this->nVersion);
  64 |          READWRITE(nExternalChainCounter);
  65 |          READWRITE(masterKeyID);
  66 | +        if (this->nVersion >= SUPPORT_WATCHONLY_VERSION) {
  67 | +            READWRITE(isWatchOnly);
  68 | +            if(isWatchOnly)
  69 | +                READWRITE(masterPubKey);

jonasschnelli commented at 4:35 PM on March 17, 2017:

Somehow this PR was crashing here because of

Assertion failed: (pubkey.size() == 33), function Encode, file pubkey.cpp, line 255.

It looks like that my tested tpub was non compressed? How is that even possible? LLDB told me pubkey = (vch = unsigned char [65] @ 0x00007fd16736976c).

jonasschnelli commented at 4:36 PM on March 17, 2017:

Wait... I was using the extended private key instead. Though, I guess it should not crash in this case.

NicolasDorier commented at 6:49 AM on March 20, 2017:

How is it possible to get an extended private key here? if you provided a xpriv instead of xpub, it should have crashed at startup.

jonasschnelli commented at 7:31 AM on March 20, 2017:

I used a tpriv which made this PR proceeding the hdwatchonly initialisation but then crashed at this point. Haven't looked into the details why the tpriv was accepted during init.

NicolasDorier commented at 7:49 AM on March 20, 2017:

ah OK I know why... I do not verify the prefix

NicolasDorier commented at 2:08 PM on March 20, 2017:

Issue fixed and tested

in src/wallet/wallet.cpp:3722 in d15d07e205 outdated

3713 | @@ -3618,14 +3714,41 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3714 |          walletInstance->SetMaxVersion(nMaxVersion);
3715 |      }
3716 |  
3717 | +
3718 | +    std::string hdWatchOnly = GetArg("-hdwatchonly", "");
3719 | +    CExtPubKey extPubKey;
3720 | +    if (!hdWatchOnly.empty()) {
3721 | +        CBitcoinExtPubKey bitcoinExtPubKey;
3722 | +        auto extpubKeyPrefix = chainparams.Base58Prefix(CChainParams::Base58Type::EXT_PUBLIC_KEY);

jonasschnelli commented at 4:44 PM on March 17, 2017:

Hmm.. I think this is incorrect. I could load a xpub (mainnet) on regtest. Shouldn't CBitcoinExtPubKey("<x|tpub>") do the check for the correct encoding according to the chainparams?

in src/wallet/wallet.cpp:3730 in d15d07e205 outdated

3725 | +            return NULL;
3726 | +        }
3727 | +        extPubKey = bitcoinExtPubKey.GetKey();
3728 | +    }
3729 | +
3730 | +    if (!fFirstRun && !hdWatchOnly.empty()) {

jonasschnelli commented at 4:47 PM on March 17, 2017:

I got no warning when I started bitcoind again with a different -hdwatchonly=xpub. Here there should be a check wether the wallet has already a watch-only xpub set, and if the user provides one, and it's different, it should warn or stop.

NicolasDorier commented at 6:44 AM on March 20, 2017:

this is normally checked. Actually I am testing it on https://github.com/bitcoin/bitcoin/pull/9728/files#diff-08fcd10ff4d7d4f9f0249ba978b3b4d4R49 this is strange... actual check done on https://github.com/bitcoin/bitcoin/pull/9728/files#diff-b2bb174788c7409b671c46ccc86034bdR3731 can you verify that you did not made another mistake ?

jonasschnelli commented at 4:47 PM on March 17, 2017: contributor

This is a great PR. I really would like this to be in 0.15. It makes HWW signing much simpler and could be a pre-step for HWW support in Core.

NicolasDorier commented at 2:15 PM on March 18, 2017: contributor

I will rebase and pass on all your nits and better test/code the case where you pass a xpriv once #9294 get merged.

NicolasDorier force-pushed on Mar 20, 2017

NicolasDorier commented at 2:10 PM on March 20, 2017: contributor

Removed the addition of CChainParams into InitLoadWallet
Correctly error if not passing an extpubkey with the right network
Testing hdwatchonly parsing

NicolasDorier force-pushed on Mar 21, 2017

fanquake added this to the milestone 0.15.0 on Mar 22, 2017

NicolasDorier force-pushed on Apr 7, 2017

NicolasDorier commented at 7:35 AM on April 7, 2017: contributor

Rebased:

Support internal addresses of #9294
Using BIP44 friendly derivation @saleemrashid

NicolasDorier force-pushed on Jun 5, 2017

NicolasDorier commented at 1:30 PM on June 5, 2017: contributor

Rebased @saleemrashid

instagibbs commented at 10:25 PM on June 15, 2017: member

needs rebase, will review

NicolasDorier force-pushed on Jun 16, 2017

NicolasDorier commented at 2:04 AM on June 16, 2017: contributor

rebased

in src/wallet/wallet.cpp:3955 in 041763e135 outdated

3932 | +        }
3933 | +        
3934 | +        if (!fFirstRun) {
3935 | +            if (!walletInstance->IsHDWatchOnly() || 
3936 | +                walletInstance->GetHDChain().masterKeyID != extPubKey.pubkey.GetID()) {
3937 | +                InitError(_("Cannot specify hdwatchonly on an already existing wallet"));

instagibbs commented at 10:20 PM on June 16, 2017:

Message should be something like "Cannot specify new hdwatchonly on an already existing wallet"

instagibbs commented at 1:54 AM on June 18, 2017: member

Is there a way exposed to the user that their wallet is HD watch-only? Might be useful in getwalletinfo.

NicolasDorier force-pushed on Jun 18, 2017

NicolasDorier commented at 5:15 AM on June 18, 2017: contributor

just added info in getwalletinfo. I sadly can't compile because of #10622 and it seems my travis build is queued and does not want to run.

in src/wallet/rpcwallet.cpp:2404 in 7ededffb3f outdated

2399 | @@ -2400,7 +2400,8 @@ UniValue getwalletinfo(const JSONRPCRequest& request)
2400 |              "  \"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"
2401 |              "  \"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"
2402 |              "  \"paytxfee\": x.xxxx,              (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n"
2403 | -            "  \"hdmasterkeyid\": \"<hash160>\"     (string) the Hash160 of the HD master pubkey\n"
2404 | +            "  \"hdmasterkeyid\": \"<hash160>\"     (string) the Hash160 of the HD master pubkey\n" + 
2405 | +            "  \"hdwatchonlykey\": \"<hdpubkey>\" (string) the HD pubkey used for key derivation in hdwatchonly mode\n"

instagibbs commented at 1:19 AM on June 19, 2017:

"extended pubkey"

in src/wallet/wallet.cpp:114 in a42aafc024 outdated

 110 | @@ -111,9 +111,7 @@ CPubKey CWallet::GenerateNewKey(bool internal)
 111 |          SetMinVersion(FEATURE_COMPRPUBKEY);
 112 |  
 113 |      if (!IsHDWatchOnly())
 114 | -    {

instagibbs commented at 1:47 AM on June 19, 2017:

Braces are the recommended code style, not sure why they're being removed.

in src/wallet/walletdb.cpp:62 in a42aafc024 outdated

  56 | @@ -57,6 +57,14 @@ bool CWalletDB::EraseTx(uint256 hash)
  57 |      return EraseIC(std::make_pair(std::string("tx"), hash));
  58 |  }
  59 |  
  60 | +bool CWalletDB::WriteKeyMeta(const CPubKey& vchPubKey, const CKeyMetadata& keyMeta)
  61 | +{
  62 | +    if (!batch.Write(std::make_pair(std::string("keymeta"), vchPubKey),

instagibbs commented at 2:08 AM on June 19, 2017:

Please add braces for readability

Should you be using WriteIC like everywhere else?

NicolasDorier commented at 2:19 AM on June 19, 2017: contributor

Ok my blinded push passed @instagibbs . Added some info in getwalletinfo as suggested.

in src/wallet/wallet.cpp:117 in 26025bc52e outdated

 115 | -    CPubKey pubkey = secret.GetPubKey();
 116 | -    assert(secret.VerifyPubKey(pubkey));
 117 | +    if (!IsHDWatchOnly())
 118 | +        assert(secret.VerifyPubKey(pubkey));
 119 |  
 120 |      mapKeyMetadata[pubkey.GetID()] = metadata;

instagibbs commented at 2:21 AM on June 19, 2017:

Perhaps move this into AddKeyPubKey

in src/wallet/wallet.cpp:113 in 26025bc52e outdated

 112 |      if (fCompressed)
 113 |          SetMinVersion(FEATURE_COMPRPUBKEY);
 114 |  
 115 | -    CPubKey pubkey = secret.GetPubKey();
 116 | -    assert(secret.VerifyPubKey(pubkey));
 117 | +    if (!IsHDWatchOnly())

instagibbs commented at 2:36 AM on June 19, 2017:

Please add braces.

in test/functional/hdwatchonly.py:58 in 26025bc52e outdated

  53 | +        # generate mine to p2pk, so let's just be sure we can solve it
  54 | +        assert_equal(unspent[1]['solvable'], True)
  55 | +
  56 | +        self.stop_nodes()
  57 | +
  58 | +        # check fails gracefully if any mess up with hdwatchonly parameter

instagibbs commented at 2:52 AM on June 19, 2017:

suggested rewording:

"check for graceful failure due to any invalid hdwatchonly parameters"

in test/functional/hdwatchonly.py:79 in 26025bc52e outdated

  74 | +        # check the hdkeypath was persisted
  75 | +        self.nodes = self.start_nodes(self.num_nodes, self.options.tmpdir, [[],[]])
  76 | +        validated_address = self.nodes[0].validateaddress('mxKeRQP6gTdCW6jHhn9FW8bGXD8W1UpR6n')
  77 | +        assert_equal(validated_address['hdkeypath'], 'm/0/1')
  78 | +
  79 | +        # check the hd key was persisted

instagibbs commented at 2:57 AM on June 19, 2017:

s/was/has/

instagibbs approved

instagibbs commented at 3:04 AM on June 19, 2017: member

tACK, works as expected

NicolasDorier force-pushed on Jun 19, 2017

NicolasDorier commented at 4:34 AM on June 19, 2017: contributor

Thanks for the review @instagibbs I addressed your nits, can you re tACK once ?

instagibbs commented at 12:10 PM on June 19, 2017: member

re-tACK https://github.com/bitcoin/bitcoin/pull/9728/commits/067b117b523208d66ade575e11ca76df964bf6ed

instagibbs commented at 12:27 PM on June 19, 2017: member

We could actually also return the xpub in validateaddress as well...

instagibbs commented at 6:04 PM on June 26, 2017: member

Included the return in validateaddress here: https://github.com/instagibbs/bitcoin/commit/b8e1316721a72fa7a0de4e9c8497746f022e9881

Please feel free to take.

Using this PR "in production" with no issues.

laanwj assigned laanwj on Jun 27, 2017

in src/wallet/wallet.cpp:1944 in 123540c7d2 outdated

1877 | +            if (!pwallet->IsHDWatchOnly() || isMine != ISMINE_WATCH_SOLVABLE)
1878 | +                return false;
1879 | +            const auto& meta = pwallet->mapKeyMetadata;
1880 | +            auto it = meta.find(CScriptID(parentOut.scriptPubKey));
1881 | +            if (it == meta.end() ||
1882 | +                it->second.hdKeypath.empty())

instagibbs commented at 4:08 PM on June 27, 2017:

I think we may need to generalize this check a bit more. If the script here is a multisig in which the watch-only wallet owns all the keys, this will return false since there is no hdKeypath.

Practically this means that spending 0-conf p2sh funds is a dicey experience that results in "Insufficient funds" responses sometimes.

Perhaps something like pwalletMain->IsHDWatchOnly(CScriptID& scriptID) to make this check, which accounts for this situation?

instagibbs commented at 4:53 PM on June 27, 2017:

Since we're not even required to be backwards compatible, I think adding a new piece of key metadata fIsHDWatchOnly, and adding that during importaddress(or some new call) when all keys are hdwatchonly keys(or a mixture of watchonly and stored private keys?).

NicolasDorier commented at 5:17 AM on June 28, 2017:

Does IsTrusted() returns ISMINE_SPENDABLE when for a multi sig output where we know all the keys when not hdwatchonly?

instagibbs commented at 12:25 PM on June 28, 2017:

If you have the private keys, yes, because of check a few lines above.

NicolasDorier commented at 4:05 AM on June 29, 2017:

So your goal is that importaddress scriptPubKeys should be considered trusted if we are in hdwatchonly mode ?

instagibbs commented at 12:21 PM on June 29, 2017:

If we completely those addresses via hdwatchonly keys and/or privkeys, yes.

instagibbs commented at 12:22 PM on June 29, 2017:

Currently in this mode you can import a privkey and spend unconfirmed outputs. Likewise you can spend unconfirmed hdwatchonly outputs. You cannot however spend a mixture of the two in a multisig, or pure hdwatchonly p2sh.

NicolasDorier force-pushed on Jun 28, 2017

NicolasDorier commented at 5:19 AM on June 28, 2017: contributor

Rebased and included @instagibbs commit.

instagibbs commented at 1:11 PM on June 29, 2017: member

Not trying to completely sidetrack this PR, but this is for future PR consideration: @NicolasDorier https://github.com/instagibbs/bitcoin/commit/bb72d12cf6f737e4e7d43a90d7446f12a7e18a60

This is the implementation that works for unconfirmed p2sh multisig where all keys are mixture of imported privkeys and hdwatchonly. Pretty ugly though and quite special-cased. Longer-term I'd look to extend ISMINE instead to have a value which covers this consideration and make this whole IsTrusted logic much simpler.

NicolasDorier commented at 1:12 PM on June 30, 2017: contributor

I think making more general case with SPENDABLE_WATCHONLY is preferable. I sadly don't have too much time these days, but I am interested into fixing it during 27-28.

in src/wallet/wallet.cpp:139 in c15332f7cb outdated

 144 | -
 145 |      // try to get the master key
 146 | -    if (!GetKey(hdChain.masterKeyID, key))
 147 | +    if (GetKey(hdChain.masterKeyID, key))
 148 | +    {
 149 | +        // for now we use a fixed keypath scheme of m/0'/0'/k

jtimon commented at 7:28 PM on July 13, 2017:

it seems like you are repeating yourself a little bit here. Perhaps a common function can be extracted to avoid some code duplication between this if and the next one?

NicolasDorier commented at 1:28 AM on July 15, 2017:

I tried to think about this, but this was not as easy. I preferred keeping this way as it makes review easier: I did not touch what was already working.

laanwj added this to the milestone 0.16.0 on Jul 17, 2017

laanwj removed this from the milestone 0.15.0 on Jul 17, 2017

NicolasDorier force-pushed on Aug 3, 2017

NicolasDorier commented at 10:41 AM on August 3, 2017: contributor

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 force-pushed on Aug 3, 2017

NicolasDorier renamed this:
~~Can create Watch Only HD wallet with -hdwatchonly~~
Can create external HD wallet with -externalhd
on Aug 3, 2017

NicolasDorier renamed this:
~~Can create external HD wallet with -externalhd~~
Can create External HD wallet with -externalhd
on Aug 3, 2017

Can create External HD wallet with -externalHD 08f613d2e9

Coins belonging to external HD are safe bcded2e607

[RPC] Add External HD key to getwalletinfo 578e8da5de

[Tests] ExternalHD tests 1cac7a66e8

NicolasDorier force-pushed on Aug 3, 2017

in src/wallet/rpcwallet.cpp:2513 in 1cac7a66e8

2509 | @@ -2510,7 +2510,8 @@ UniValue getwalletinfo(const JSONRPCRequest& request)
2510 |              "  \"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"
2511 |              "  \"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"
2512 |              "  \"paytxfee\": x.xxxx,              (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n"
2513 | -            "  \"hdmasterkeyid\": \"<hash160>\"     (string) the Hash160 of the HD master pubkey\n"
2514 | +            "  \"hdmasterkeyid\": \"<hash160>\"   (string) the Hash160 of the HD master pubkey\n" +

jonasschnelli commented at 7:57 PM on August 15, 2017:

nit: missing two whitespaces before (string).

in src/wallet/wallet.cpp:126 in 1cac7a66e8

 128 | +        if (!AddKeyPubKeyWithDB(walletdb, secret, pubkey)) {
 129 | +            throw std::runtime_error(std::string(__func__) + ": AddKey failed");
 130 | +        }
 131 | +    }
 132 | +    else {
 133 | +        if (!AddWatchOnly(pubkey, metadata, nCreationTime))

jonasschnelli commented at 7:58 PM on August 15, 2017:

nit: brackets

in src/wallet/wallet.cpp:190 in 1cac7a66e8

 200 | +
 201 | +        // derive child key at next index, skip keys already known to the wallet
 202 | +        do {
 203 | +            if (internal) {
 204 | +                chainChildKey.Derive(childKey, hdChain.nInternalChainCounter);
 205 | +                metadata.hdKeypath = "m/1/" + std::to_string(hdChain.nInternalChainCounter);

jonasschnelli commented at 8:01 PM on August 15, 2017:

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 commented at 8:07 PM on August 15, 2017: contributor

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 commented at 8:07 PM on August 15, 2017: contributor

Needs rebase.

NicolasDorier commented at 2:39 AM on August 17, 2017: contributor

@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 commented at 5:56 AM on October 30, 2017: contributor

What is the status of this?

NicolasDorier commented at 7:34 PM on October 31, 2017: contributor

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 commented at 7:57 PM on November 6, 2017: member

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

laanwj removed this from the milestone 0.16.0 on Jan 11, 2018

laanwj added this to the milestone 0.17.0 on Jan 11, 2018

jonasschnelli commented at 10:54 AM on February 17, 2018: contributor

Rebase required...

NicolasDorier commented at 3:02 PM on February 17, 2018: contributor

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

instagibbs commented at 3:09 PM on February 17, 2018: member

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

NicolasDorier commented at 3:18 PM on February 17, 2018: contributor

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 commented at 5:08 PM on March 6, 2018: contributor

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.

NicolasDorier closed this on Mar 6, 2018

MarcoFalke locked this on Sep 8, 2021