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.

Sign up for GitHub

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

Jump to bottom

Can create External HD wallet with -externalhd #9728

Closed
wants to merge 4 commits into from
Closed

Can create External HD wallet with -externalhd #9728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
08f613d
Can create External HD wallet with -externalHD
NicolasDorier Feb 9, 2017
bcded2e
Coins belonging to external HD are safe
NicolasDorier Feb 22, 2017
578e8da
[RPC] Add External HD key to getwalletinfo
NicolasDorier Jun 18, 2017
1cac7a6
[Tests] ExternalHD tests
NicolasDorier Mar 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/base58.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ template<typename K, int Size, CChainParams::Base58Type Type> class CBitcoinExtK

K GetKey() {
K ret;
if (vchData.size() == Size) {
const bool fCorrectVersion = vchVersion == Params().Base58Prefix(Type);
if (vchData.size() == Size && fCorrectVersion) {
// If base58 encoded data does not hold an ext key, return a !IsValid() key
ret.Decode(vchData.data());
}
Expand Down
10 changes: 10 additions & 0 deletions src/keystore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,16 @@ bool CBasicKeyStore::HaveWatchOnly(const CScript &dest) const
return setWatchOnly.count(dest) > 0;
}

bool CBasicKeyStore::HaveWatchOnly(const CKeyID &keyId) const
{
LOCK(cs_KeyStore);
WatchKeyMap::const_iterator it = mapWatchKeys.find(keyId);
if (it != mapWatchKeys.end()) {
return true;
}
return false;
}

bool CBasicKeyStore::HaveWatchOnly() const
{
LOCK(cs_KeyStore);
Expand Down
2 changes: 2 additions & 0 deletions src/keystore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ class CBasicKeyStore : public CKeyStore
virtual bool AddWatchOnly(const CScript &dest) override;
virtual bool RemoveWatchOnly(const CScript &dest) override;
virtual bool HaveWatchOnly(const CScript &dest) const override;

virtual bool HaveWatchOnly(const CKeyID &keyId) const;
virtual bool HaveWatchOnly() const override;
};

Expand Down
5 changes: 5 additions & 0 deletions src/rpc/misc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,11 @@ UniValue validateaddress(const JSONRPCRequest& request)
ret.push_back(Pair("hdmasterkeyid", it->second.hdMasterKeyID.GetHex()));
}
}

if (pwallet->IsExternalHD()) {
CBitcoinExtPubKey externalHD(pwallet->GetHDChain().externalHD);
ret.push_back(Pair("externalhdkey", externalHD.ToString()));
}
}
#endif
}
Expand Down
7 changes: 6 additions & 1 deletion src/wallet/rpcwallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" +
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing two whitespaces before (string).

" \"externalhdkey\": \"<hdpubkey>\" (string) the extended pubkey used for key derivation in external HD mode\n"
"}\n"
"\nExamples:\n"
+ HelpExampleCli("getwalletinfo", "")
Expand All @@ -2537,6 +2538,10 @@ UniValue getwalletinfo(const JSONRPCRequest& request)
if (pwallet->IsCrypted()) {
obj.push_back(Pair("unlocked_until", pwallet->nRelockTime));
}
if (pwallet->IsExternalHD()) {
CBitcoinExtPubKey watchOnly(pwallet->GetHDChain().externalHD);
obj.push_back(Pair("externalhdkey", watchOnly.ToString()));
}
obj.push_back(Pair("paytxfee", ValueFromAmount(payTxFee.GetFeePerK())));
if (!masterKeyID.IsNull())
obj.push_back(Pair("hdmasterkeyid", masterKeyID.GetHex()));
Expand Down
232 changes: 179 additions & 53 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,79 +95,116 @@ CPubKey CWallet::GenerateNewKey(CWalletDB &walletdb, bool internal)
bool fCompressed = CanSupportFeature(FEATURE_COMPRPUBKEY); // default to compressed public keys if we want 0.6.0 wallets

CKey secret;

CPubKey pubkey;
// Create new metadata
int64_t nCreationTime = GetTime();
CKeyMetadata metadata(nCreationTime);

// use HD key derivation if HD was enabled during wallet creation
if (IsHDEnabled()) {
DeriveNewChildKey(walletdb, metadata, secret, (CanSupportFeature(FEATURE_HD_SPLIT) ? internal : false));
DeriveNewChildKey(walletdb, metadata, secret, pubkey, (CanSupportFeature(FEATURE_HD_SPLIT) ? internal : false));
} else {
secret.MakeNewKey(fCompressed);
pubkey = secret.GetPubKey();
assert(secret.VerifyPubKey(pubkey));
}

// Compressed public keys were introduced in version 0.6.0
if (fCompressed) {
SetMinVersion(FEATURE_COMPRPUBKEY);
}

CPubKey pubkey = secret.GetPubKey();
assert(secret.VerifyPubKey(pubkey));

mapKeyMetadata[pubkey.GetID()] = metadata;
UpdateTimeFirstKey(nCreationTime);

if (!AddKeyPubKeyWithDB(walletdb, secret, pubkey)) {
throw std::runtime_error(std::string(__func__) + ": AddKey failed");
if(!IsExternalHD()) {
if (!AddKeyPubKeyWithDB(walletdb, secret, pubkey)) {
throw std::runtime_error(std::string(__func__) + ": AddKey failed");
}
}
else {
if (!AddWatchOnly(pubkey, metadata, nCreationTime))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: brackets

throw std::runtime_error(std::string(__func__) + ": AddWatchOnly failed");
}
return pubkey;
}

void CWallet::DeriveNewChildKey(CWalletDB &walletdb, CKeyMetadata& metadata, CKey& secret, bool internal)

void CWallet::DeriveNewChildKey(CWalletDB &walletdb, CKeyMetadata& metadata, CKey& secret, CPubKey& pubkey, bool internal)
{
// for now we use a fixed keypath scheme of m/0'/0'/k
CKey key; //master key seed (256bit)
CExtKey masterKey; //hd master key
CExtKey accountKey; //key at m/0'
CExtKey chainChildKey; //key at m/0'/0' (external) or m/0'/1' (internal)
CExtKey childKey; //key at m/0'/0'/<n>'

// try to get the master key
if (!GetKey(hdChain.masterKeyID, key))
if (GetKey(hdChain.masterKeyID, key))
{
// for now we use a fixed keypath scheme of m/0'/0'/k
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

CExtKey masterKey; //hd master key
CExtKey accountKey; //key at m/0'
CExtKey chainChildKey; //key at m/0'/0' (external) or m/0'/1' (internal)
CExtKey childKey; //key at m/0'/0'/<n>'

masterKey.SetMaster(key.begin(), key.size());

// derive m/0'
// use hardened derivation (child keys >= 0x80000000 are hardened after bip32)
masterKey.Derive(accountKey, BIP32_HARDENED_KEY_LIMIT);

// derive m/0'/0' (external chain) OR m/0'/1' (internal chain)
assert(internal ? CanSupportFeature(FEATURE_HD_SPLIT) : true);
accountKey.Derive(chainChildKey, BIP32_HARDENED_KEY_LIMIT + (internal ? 1 : 0));

// derive child key at next index, skip keys already known to the wallet
do {
// always derive hardened keys
// childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range
// example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649
if (internal) {
chainChildKey.Derive(childKey, hdChain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
metadata.hdKeypath = "m/0'/1'/" + std::to_string(hdChain.nInternalChainCounter) + "'";
hdChain.nInternalChainCounter++;
}
else {
chainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
metadata.hdKeypath = "m/0'/0'/" + std::to_string(hdChain.nExternalChainCounter) + "'";
hdChain.nExternalChainCounter++;
}
} while (HaveKey(childKey.key.GetPubKey().GetID()));
secret = childKey.key;
pubkey = childKey.key.GetPubKey();
metadata.hdMasterKeyID = hdChain.masterKeyID;
// update the chain model in the database
if (!walletdb.WriteHDChain(hdChain))
throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed");
}
else if (IsExternalHD()) {
CExtPubKey& masterKey = hdChain.externalHD; //hd master key
CExtPubKey chainChildKey; //key at m/0 (external) or m/1 (internal)
CExtPubKey childKey; //key at m/0/<n>

// derive m/x
masterKey.Derive(chainChildKey, internal ? 1 : 0);

// derive child key at next index, skip keys already known to the wallet
do {
if (internal) {
chainChildKey.Derive(childKey, hdChain.nInternalChainCounter);
metadata.hdKeypath = "m/1/" + std::to_string(hdChain.nInternalChainCounter);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

hdChain.nInternalChainCounter++;
}
else {
chainChildKey.Derive(childKey, hdChain.nExternalChainCounter);
metadata.hdKeypath = "m/0/" + std::to_string(hdChain.nExternalChainCounter);
hdChain.nExternalChainCounter++;
}
metadata.hdMasterKeyID = hdChain.masterKeyID;
} while (HaveWatchOnly(childKey.pubkey.GetID()));
pubkey = childKey.pubkey;
// update the chain model in the database
if (!walletdb.WriteHDChain(hdChain))
throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed");
}
else {
throw std::runtime_error(std::string(__func__) + ": Master key not found");

masterKey.SetMaster(key.begin(), key.size());

// derive m/0'
// use hardened derivation (child keys >= 0x80000000 are hardened after bip32)
masterKey.Derive(accountKey, BIP32_HARDENED_KEY_LIMIT);

// derive m/0'/0' (external chain) OR m/0'/1' (internal chain)
assert(internal ? CanSupportFeature(FEATURE_HD_SPLIT) : true);
accountKey.Derive(chainChildKey, BIP32_HARDENED_KEY_LIMIT+(internal ? 1 : 0));

// derive child key at next index, skip keys already known to the wallet
do {
// always derive hardened keys
// childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range
// example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649
if (internal) {
chainChildKey.Derive(childKey, hdChain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
metadata.hdKeypath = "m/0'/1'/" + std::to_string(hdChain.nInternalChainCounter) + "'";
hdChain.nInternalChainCounter++;
}
else {
chainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
metadata.hdKeypath = "m/0'/0'/" + std::to_string(hdChain.nExternalChainCounter) + "'";
hdChain.nExternalChainCounter++;
}
} while (HaveKey(childKey.key.GetPubKey().GetID()));
secret = childKey.key;
metadata.hdMasterKeyID = hdChain.masterKeyID;
// update the chain model in the database
if (!walletdb.WriteHDChain(hdChain))
throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed");
}
}

bool CWallet::AddKeyPubKeyWithDB(CWalletDB &walletdb, const CKey& secret, const CPubKey &pubkey)
Expand Down Expand Up @@ -298,6 +335,29 @@ bool CWallet::AddWatchOnly(const CScript& dest, int64_t nCreateTime)
return AddWatchOnly(dest);
}

bool CWallet::AddWatchOnly(const CPubKey &pubkey, const CKeyMetadata& meta, int64_t nCreateTime)
{
auto script = GetScriptForDestination(pubkey.GetID());
if (!HaveWatchOnly(script))
{
mapKeyMetadata[CScriptID(script)] = meta;
if (!AddWatchOnly(script, nCreateTime))
return false;
}

if (!CWalletDB(*dbw).WriteKeyMeta(pubkey, meta))
return false;

script = GetScriptForRawPubKey(pubkey);
if (!HaveWatchOnly(script))
{
mapKeyMetadata[CScriptID(script)] = meta;
if (!AddWatchOnly(script, nCreateTime))
return false;
}
return true;
}

bool CWallet::RemoveWatchOnly(const CScript &dest)
{
AssertLockHeld(cs_wallet);
Expand Down Expand Up @@ -1393,6 +1453,25 @@ bool CWallet::SetHDMasterKey(const CPubKey& pubkey)
return true;
}

bool CWallet::SetExternalHD(const CExtPubKey& extPubKey)
{
LOCK(cs_wallet);

// ensure this wallet.dat can only be opened by clients supporting HD
SetMinVersion(FEATURE_EXTERNAL_HD);

// store the keyid (hash160) together with
// the child index counter in the database
// as a hdchain object
CHDChain newHdChain;
newHdChain.masterKeyID = extPubKey.pubkey.GetID();
newHdChain.isExternalHD = true;
newHdChain.externalHD = extPubKey;
SetHDChain(newHdChain, false);

return true;
}

bool CWallet::SetHDChain(const CHDChain& chain, bool memonly)
{
LOCK(cs_wallet);
Expand All @@ -1408,6 +1487,11 @@ bool CWallet::IsHDEnabled() const
return !hdChain.masterKeyID.IsNull();
}

bool CWallet::IsExternalHD() const
{
return IsHDEnabled() && hdChain.isExternalHD;
}

int64_t CWalletTx::GetTxTime() const
{
int64_t n = nTimeSmart;
Expand Down Expand Up @@ -1848,8 +1932,18 @@ bool CWalletTx::IsTrusted() const
if (parent == NULL)
return false;
const CTxOut& parentOut = parent->tx->vout[txin.prevout.n];
if (pwallet->IsMine(parentOut) != ISMINE_SPENDABLE)
return false;
const auto& isMine = pwallet->IsMine(parentOut);
if (isMine != ISMINE_SPENDABLE)
{
// If the wallet is external HD, check if it is a key we generated
if (!pwallet->IsExternalHD() || isMine != ISMINE_WATCH_SOLVABLE)
return false;
const auto& meta = pwallet->mapKeyMetadata;
auto it = meta.find(CScriptID(parentOut.scriptPubKey));
if (it == meta.end() ||
it->second.hdKeypath.empty())
Copy link
Member

@instagibbs instagibbs Jun 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@NicolasDorier NicolasDorier Jun 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

return false;
}
}
return true;
}
Expand Down Expand Up @@ -3290,9 +3384,14 @@ void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRe
if (!walletdb.ReadPool(nIndex, keypool)) {
throw std::runtime_error(std::string(__func__) + ": read failed");
}
if (!HaveKey(keypool.vchPubKey.GetID())) {
if (!IsExternalHD() && !HaveKey(keypool.vchPubKey.GetID())) {
throw std::runtime_error(std::string(__func__) + ": unknown key in key pool");
}
if (IsExternalHD()) {
if (!HaveWatchOnly(keypool.vchPubKey.GetID())) {
throw std::runtime_error(std::string(__func__) + ": unknown key in key pool");
}
}
if (keypool.fInternal != fReturningInternal) {
throw std::runtime_error(std::string(__func__) + ": keypool entry misclassified");
}
Expand Down Expand Up @@ -3855,6 +3954,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), DEFAULT_SPEND_ZEROCONF_CHANGE));
strUsage += HelpMessageOpt("-txconfirmtarget=<n>", strprintf(_("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)"), DEFAULT_TX_CONFIRM_TARGET));
strUsage += HelpMessageOpt("-usehd", _("Use hierarchical deterministic key generation (HD) after BIP32. Only has effect during wallet creation/first start") + " " + strprintf(_("(default: %u)"), DEFAULT_USE_HD_WALLET));
strUsage += HelpMessageOpt("-externalhd", _("Create a new external-HD wallet from a BIP32 HD public key"));
strUsage += HelpMessageOpt("-walletrbf", strprintf(_("Send transactions with full-RBF opt-in enabled (default: %u)"), DEFAULT_WALLET_RBF));
strUsage += HelpMessageOpt("-upgradewallet", _("Upgrade wallet to latest format on startup"));
strUsage += HelpMessageOpt("-wallet=<file>", _("Specify wallet file (within data directory)") + " " + strprintf(_("(default: %s)"), DEFAULT_WALLET_DAT));
Expand Down Expand Up @@ -3949,6 +4049,26 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
walletInstance->SetMaxVersion(nMaxVersion);
}


std::string externalHd = GetArg("-externalhd", "");
CExtPubKey extPubKey;
if (!externalHd.empty()) {
CBitcoinExtPubKey bitcoinExtPubKey(externalHd);
extPubKey = bitcoinExtPubKey.GetKey();
if (!extPubKey.pubkey.IsFullyValid()) {
InitError(_("Invalid ExtPubKey format"));
return NULL;
}

if (!fFirstRun) {
if (!walletInstance->IsExternalHD() ||
walletInstance->GetHDChain().masterKeyID != extPubKey.pubkey.GetID()) {
InitError(_("Cannot specify new external hd on an already existing wallet"));
return NULL;
}
}
}

if (fFirstRun)
{
// Create new keyUser and set as default key
Expand All @@ -3958,9 +4078,15 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
walletInstance->SetMinVersion(FEATURE_HD_SPLIT);

// generate a new master key
CPubKey masterPubKey = walletInstance->GenerateNewHDMasterKey();
if (!walletInstance->SetHDMasterKey(masterPubKey))
throw std::runtime_error(std::string(__func__) + ": Storing master key failed");
if(externalHd.empty()) {
CPubKey masterPubKey = walletInstance->GenerateNewHDMasterKey();
if (!walletInstance->SetHDMasterKey(masterPubKey))
throw std::runtime_error(std::string(__func__) + ": Storing master key failed");
}
else {
if (!walletInstance->SetExternalHD(extPubKey))
throw std::runtime_error(std::string(__func__) + ": Storing master key failed");
}
}
CPubKey newDefaultKey;
if (walletInstance->GetKeyFromPool(newDefaultKey, false)) {
Expand Down
Loading