From a67fb00d0a46e0452515d7cb7a8e94194649c3de Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 3 Nov 2021 16:36:42 -0600 Subject: [PATCH] Apply suggestions from code review Co-authored-by: str4d --- src/gtest/test_keystore.cpp | 1 + src/test/alert_tests.cpp | 2 +- src/wallet/asyncrpcoperation_saplingmigration.cpp | 5 ++--- src/wallet/crypter.cpp | 6 ++++-- src/wallet/gtest/test_wallet_zkeys.cpp | 9 +++++---- src/wallet/rpcdump.cpp | 4 ++-- src/wallet/rpcwallet.cpp | 3 +-- src/wallet/wallet.cpp | 13 +++++-------- src/wallet/wallet.h | 6 +++--- src/wallet/walletdb.cpp | 14 +++++++------- src/zcash/address/zip32.cpp | 11 +++++++---- src/zcash/address/zip32.h | 4 ++-- 12 files changed, 40 insertions(+), 38 deletions(-) diff --git a/src/gtest/test_keystore.cpp b/src/gtest/test_keystore.cpp index e3473806f..c9b230b59 100644 --- a/src/gtest/test_keystore.cpp +++ b/src/gtest/test_keystore.cpp @@ -53,6 +53,7 @@ TEST(KeystoreTests, StoreAndRetrieveLegacyHDSeed) { EXPECT_FALSE(seedOut.has_value()); // Generate a random seed + // (We use MnemonicSeed purely to generate a seed, and then drop the mnemonic part.) HDSeed seed = MnemonicSeed::Random(SLIP44_TESTNET_TYPE); // We should be able to set and retrieve the seed diff --git a/src/test/alert_tests.cpp b/src/test/alert_tests.cpp index fbfc7fa16..ba4fae55e 100644 --- a/src/test/alert_tests.cpp +++ b/src/test/alert_tests.cpp @@ -117,7 +117,7 @@ bool SignAlert(CAlert &alert) // sign alert std::vector vchTmp(ParseHex(pszPrivKey)); CPrivKey vchPrivKey(vchTmp.begin(), vchTmp.end()); - std::optional key = CKey::FromPrivKey(SetPrivKey(vchPrivKey, false); + std::optional key = CKey::FromPrivKey(vchPrivKey, false); if (!key.has_value()) { printf("key.SetPrivKey failed\n"); return false; diff --git a/src/wallet/asyncrpcoperation_saplingmigration.cpp b/src/wallet/asyncrpcoperation_saplingmigration.cpp index db6ae3a3d..ee2cdcb9a 100644 --- a/src/wallet/asyncrpcoperation_saplingmigration.cpp +++ b/src/wallet/asyncrpcoperation_saplingmigration.cpp @@ -191,9 +191,8 @@ CAmount AsyncRPCOperation_saplingmigration::chooseAmount(const CAmount& availabl return amount; } -// Unless otherwise specified, the migration destination address is the address -// the legacy Sapling account (0x7FFFFFFF) at the smallest diversifier index -// that produces a valid diversified address. +// Unless otherwise specified, the migration destination address is the +// default address for the key at m/32'/coin_type'/0x7FFFFFFF'/0' libzcash::SaplingPaymentAddress AsyncRPCOperation_saplingmigration::getMigrationDestAddress(const HDSeed& seed) { KeyIO keyIO(Params()); if (mapArgs.count("-migrationdestaddress")) { diff --git a/src/wallet/crypter.cpp b/src/wallet/crypter.cpp index 0439ec480..5ef7cabf3 100644 --- a/src/wallet/crypter.cpp +++ b/src/wallet/crypter.cpp @@ -372,7 +372,8 @@ bool CCryptoKeyStore::SetCryptedMnemonicSeed( { LOCK(cs_KeyStore); if (!fUseCrypto || !cryptedMnemonicSeed.first.IsNull()) { - // Require encryption & don't allow an existing seed to be changed. + // Either wallet encryption is disabled, or the wallet already has an + // existing mnemonic. In either case, don't allow a new mnemonic. return false; } else { cryptedMnemonicSeed = std::make_pair(seedFp, vchCryptedSecret); @@ -439,7 +440,8 @@ bool CCryptoKeyStore::SetCryptedLegacyHDSeed( { LOCK(cs_KeyStore); if (!fUseCrypto || cryptedLegacySeed.has_value()) { - // Require encryption & don't allow an existing seed to be changed. + // Either wallet encryption is disabled, or the wallet already has an + // existing legacy seed. In either case, don't allow a new seed. return false; } else { cryptedLegacySeed = std::make_pair(seedFp, vchCryptedSecret); diff --git a/src/wallet/gtest/test_wallet_zkeys.cpp b/src/wallet/gtest/test_wallet_zkeys.cpp index 538f6ebcd..4e2bc5abb 100644 --- a/src/wallet/gtest/test_wallet_zkeys.cpp +++ b/src/wallet/gtest/test_wallet_zkeys.cpp @@ -65,9 +65,10 @@ TEST(WalletZkeysTest, StoreAndLoadSaplingZkeys) { EXPECT_EQ(1, addrs.count(address)); EXPECT_EQ(1, addrs.count(sk.ToXFVK().DefaultAddress())); - // Generate a diversified address different to the default - // If we can't get an early diversified address, we are very unlucky - libzcash::diversifier_index_t j(2); + // Find a diversified address that does not use the same diversifier as the default address. + // By starting our search at `10` we ensure there's no more than a 2^-10 chance that we + // collide with the default diversifier. + libzcash::diversifier_index_t j(10); auto dpa = sk.ToXFVK().FindAddress(j).first; // verify wallet only has the default address @@ -448,7 +449,7 @@ TEST(WalletZkeysTest, WriteCryptedSaplingZkeyDirectToDb) { // If we can't get an early diversified address, we are very unlucky libzcash::SaplingExtendedSpendingKey extsk; EXPECT_TRUE(wallet.GetSaplingExtendedSpendingKey(address, extsk)); - libzcash::diversifier_index_t j(2); + libzcash::diversifier_index_t j(10); auto dpa = extsk.ToXFVK().FindAddress(j).first; // Add diversified address to the wallet diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 153024bf5..4e302c95a 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -614,7 +614,7 @@ UniValue dumpwallet_impl(const UniValue& params, bool fDumpZKeys) if (hdSeed.has_value()) { auto mSeed = hdSeed.value(); file << strprintf( - "# Emergency Recovery Information\n" + "# Emergency Recovery Information:\n" "# - recovery_phrase=\"%s\"\n" "# - language=%s\n" "# - fingerprint=%s\n", @@ -628,7 +628,7 @@ UniValue dumpwallet_impl(const UniValue& params, bool fDumpZKeys) if (legacySeed.has_value()) { auto rawSeed = legacySeed.value().RawSeed(); file << strprintf( - "# Legacy HD Seed\n" + "# Legacy HD Seed:\n" "# - seed=%s\n" "# - fingerprint=%s\n", HexStr(rawSeed.begin(), rawSeed.end()), diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 3c27e0eeb..090459fc2 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -54,7 +54,6 @@ using namespace libzcash; const std::string ADDR_TYPE_SPROUT = "sprout"; const std::string ADDR_TYPE_SAPLING = "sapling"; -const bool DEFAULT_WALLET_REQUIRE_BACKUP = true; extern UniValue TxJoinSplitToJSON(const CTransaction& tx); @@ -89,7 +88,7 @@ void EnsureWalletIsBackedUp(const CChainParams& params) throw JSONRPCError( RPC_WALLET_BACKUP_REQUIRED, "Error: Please acknowledge that you have backed up the wallet's emergency recovery phrase " - "with walletconfirmbackup first." + "by using the walletconfirmbackup RPC method first." ); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5dc58e87e..716b72061 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -110,8 +110,8 @@ libzcash::SproutPaymentAddress CWallet::GenerateNewSproutZKey() return addr; } -// Generate a new Sapling spending key as a child of the legacy Sapling account -// return its public payment address. +// Generates a new Sapling spending key as a child of the legacy Sapling account, +// and returns its public payment address. // // The z_getnewaddress API must use the mnemonic HD seed, and fail if that seed // is not present. The account index is determined by trial of values of @@ -435,7 +435,7 @@ ZcashdUnifiedSpendingKey CWallet::GenerateNewUnifiedSpendingKey() { std::optional CWallet::GenerateUnifiedSpendingKeyForAccount(uint32_t accountId) { auto seed = GetMnemonicSeed(); if (!seed.has_value()) { - throw std::runtime_error(std::string(__func__) + ": Wallet has no mnemonic HD seed. Please upgrade this wallet."); + throw std::runtime_error(std::string(__func__) + ": Wallet has no mnemonic HD seed."); } auto usk = ZcashdUnifiedSpendingKey::ForAccount(seed.value(), BIP44CoinType(), accountId); @@ -4240,8 +4240,7 @@ bool CWallet::NewKeyPool() for (int i = 0; i < nKeys; i++) { int64_t nIndex = i+1; - auto key = GenerateNewKey(); - walletdb.WritePool(nIndex, CKeyPool(key)); + walletdb.WritePool(nIndex, CKeyPool(GenerateNewKey())); setKeyPool.insert(nIndex); } LogPrintf("CWallet::NewKeyPool wrote %d new keys\n", nKeys); @@ -4271,8 +4270,7 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) int64_t nEnd = 1; if (!setKeyPool.empty()) nEnd = *(--setKeyPool.end()) + 1; - auto newKey = GenerateNewKey(); - if (!walletdb.WritePool(nEnd, CKeyPool(newKey))) + if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey()))) throw runtime_error("TopUpKeyPool(): writing generated key failed"); setKeyPool.insert(nEnd); LogPrintf("keypool added key %d, size=%u\n", nEnd, setKeyPool.size()); @@ -4898,7 +4896,6 @@ bool CWallet::InitLoadWallet(const CChainParams& params, bool clearWitnessCaches walletInstance->SetMaxVersion(nMaxVersion); } - // TODO: should this go in `LoadWallet` instead? if (!walletInstance->HaveMnemonicSeed()) { // We can't set the new HD seed until the wallet is decrypted. diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 38c2e5c83..16dd7141b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1060,7 +1060,7 @@ public: libzcash::SaplingPaymentAddress GenerateNewLegacySaplingZKey(); //! Generates Sapling key at the specified address index, and stores the newly generated //! spending key to the wallet if it has not alreay been persisted. - //! Returns the newly created key, its metadata, and a flag distinguishing + //! Returns the newly created key, and a flag distinguishing //! whether or not the key was already known by the wallet. std::pair GenerateLegacySaplingZKey(uint32_t addrIndex); //! Adds Sapling spending key to the store, and saves it to disk @@ -1308,12 +1308,12 @@ public: bool VerifyMnemonicSeed(const SecureString& mnemonic); bool MnemonicVerified(); - /* Set the current HD seed, without saving it to disk (used by LoadWallet) */ + /* Set the current mnemonic phrase, without saving it to disk (used by LoadWallet) */ bool LoadMnemonicSeed(const MnemonicSeed& seed); /* Set the legacy HD seed, without saving it to disk (used by LoadWallet) */ bool LoadLegacyHDSeed(const HDSeed& seed); - /* Set the current encrypted HD seed, without saving it to disk (used by LoadWallet) */ + /* Set the current encrypted mnemonic phrase, without saving it to disk (used by LoadWallet) */ bool LoadCryptedMnemonicSeed(const uint256& seedFp, const std::vector& seed); /* Set the legacy encrypted HD seed, without saving it to disk (used by LoadWallet) */ bool LoadCryptedLegacyHDSeed(const uint256& seedFp, const std::vector& seed); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 417a5f8d8..dfe8ebf69 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -685,7 +685,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, { ssValue >> pwallet->nWitnessCacheSize; } - else if (strType == "mnemonicseed") + else if (strType == "mnemonicphrase") { uint256 seedFp; ssKey >> seedFp; @@ -693,17 +693,17 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, if (seed.Fingerprint() != seedFp) { - strErr = "Error reading wallet database: HDSeed corrupt"; + strErr = "Error reading wallet database: mnemonic phrase corrupt"; return false; } if (!pwallet->LoadMnemonicSeed(seed)) { - strErr = "Error reading wallet database: LoadHDSeed failed"; + strErr = "Error reading wallet database: LoadMnemonicSeed failed"; return false; } } - else if (strType == "chdmnemonicseed") + else if (strType == "cmnemonicphrase") { uint256 seedFp; vector vchCryptedSecret; @@ -774,7 +774,7 @@ static bool IsKeyType(string strType) { return (strType== "key" || strType == "wkey" || strType == "hdseed" || strType == "chdseed" || - strType == "mnemonicseed" || strType == "chdmnemonicseed" || + strType == "mnemonicphrase" || strType == "cmnemonicphrase" || strType == "zkey" || strType == "czkey" || strType == "sapzkey" || strType == "csapzkey" || strType == "vkey" || strType == "sapextfvk" || @@ -1194,13 +1194,13 @@ bool CWalletDB::WriteNetworkInfo(const std::string& networkId) bool CWalletDB::WriteMnemonicSeed(const MnemonicSeed& seed) { nWalletDBUpdateCounter++; - return Write(std::make_pair(std::string("mnemonicseed"), seed.Fingerprint()), seed); + return Write(std::make_pair(std::string("mnemonicphrase"), seed.Fingerprint()), seed); } bool CWalletDB::WriteCryptedMnemonicSeed(const uint256& seedFp, const std::vector& vchCryptedSecret) { nWalletDBUpdateCounter++; - return Write(std::make_pair(std::string("chdmnemonicseed"), seedFp), vchCryptedSecret); + return Write(std::make_pair(std::string("cmnemonicphrase"), seedFp), vchCryptedSecret); } bool CWalletDB::WriteMnemonicHDChain(const CHDChain& chain) diff --git a/src/zcash/address/zip32.cpp b/src/zcash/address/zip32.cpp index c804c5aba..180a5872a 100644 --- a/src/zcash/address/zip32.cpp +++ b/src/zcash/address/zip32.cpp @@ -25,7 +25,7 @@ MnemonicSeed MnemonicSeed::Random(uint32_t bip44CoinType, Language language, siz { assert(entropyLen >= 32); while (true) { // loop until we find usable entropy - std::vector entropy(entropyLen, 0); + RawHDSeed entropy(entropyLen, 0); GetRandBytes(entropy.data(), entropyLen); const char* phrase = zip339_entropy_to_phrase(language, entropy.data(), entropyLen); SecureString mnemonic(phrase); @@ -99,7 +99,7 @@ std::optional> DeriveBip44TransparentAccountKey(co } std::optional Bip44AccountChains::ForAccount( - const HDSeed& seed, + const MnemonicSeed& seed, uint32_t bip44CoinType, uint32_t accountId) { auto accountKeyOpt = DeriveBip44TransparentAccountKey(seed, bip44CoinType, accountId); @@ -263,7 +263,10 @@ std::pair SaplingExtendedSpendingKey::For std::pair SaplingExtendedSpendingKey::Legacy(const HDSeed& seed, uint32_t bip44CoinType, uint32_t addressIndex) { auto m = Master(seed); - // We use a fixed keypath scheme of m/32'/coin_type'/account'/addressIndex' + // We use a fixed keypath scheme of m/32'/coin_type'/0x7FFFFFFF'/addressIndex' + // This is not a "standard" path, but instead is a predictable location for + // legacy zcashd-derived keys that is minimally different from the UA account + // path, while unlikely to collide with normal UA account usage. // Derive m/32' auto m_32h = m.Derive(32 | ZIP32_HARDENED_KEY_LIMIT); @@ -301,7 +304,7 @@ SaplingExtendedFullViewingKey SaplingExtendedSpendingKey::ToXFVK() const // Unified // -std::optional> ZcashdUnifiedSpendingKey::ForAccount(const HDSeed& seed, uint32_t bip44CoinType, uint32_t accountId) { +std::optional> ZcashdUnifiedSpendingKey::ForAccount(const MnemonicSeed& seed, uint32_t bip44CoinType, uint32_t accountId) { ZcashdUnifiedSpendingKey usk; usk.accountId = accountId; diff --git a/src/zcash/address/zip32.h b/src/zcash/address/zip32.h index 365c3654f..7e8697cc0 100644 --- a/src/zcash/address/zip32.h +++ b/src/zcash/address/zip32.h @@ -367,7 +367,7 @@ private: ZcashdUnifiedSpendingKey() {} public: static std::optional> ForAccount( - const HDSeed& seed, + const MnemonicSeed& mnemonic, uint32_t bip44CoinType, uint32_t accountId); @@ -396,7 +396,7 @@ private: seedFp(seedFpIn), accountId(accountIdIn), bip44CoinType(bip44CoinTypeIn), external(externalIn), internal(internalIn) {} public: static std::optional ForAccount( - const HDSeed& seed, + const MnemonicSeed& mnemonic, uint32_t bip44CoinType, uint32_t accountId);