diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 16118ad10..7c1d81660 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -266,15 +266,15 @@ uint256 AsyncRPCOperation_sendmany::main_impl() { std::visit(match { [&](const CKeyID& keyId) { auto accountId = selectorAccountId.value_or(ZCASH_LEGACY_ACCOUNT); - builder_.SendChangeTo( - pwalletMain->GenerateChangeAddressForAccount(accountId, { libzcash::ChangeType::Sapling }).value(), - ovks.first); + auto changeAddr = pwalletMain->GenerateChangeAddressForAccount( + accountId, { libzcash::ChangeType::Transparent }).value(); + builder_.SendChangeTo(changeAddr, ovks.first); }, [&](const CScriptID& scriptId) { auto accountId = selectorAccountId.value_or(ZCASH_LEGACY_ACCOUNT); - builder_.SendChangeTo( - pwalletMain->GenerateChangeAddressForAccount(accountId, { libzcash::ChangeType::Sapling }).value(), - ovks.first); + auto changeAddr = pwalletMain->GenerateChangeAddressForAccount( + accountId, { libzcash::ChangeType::Transparent }).value(); + builder_.SendChangeTo(changeAddr, ovks.first); }, [&](const libzcash::SproutPaymentAddress& addr) { // for Sprout, we return change to the originating address. diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 6a84c89de..dd836441b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4010,18 +4010,15 @@ UniValue z_viewtransaction(const UniValue& params, bool fHelp) // Collect OutgoingViewingKeys for recovering output information std::set ovks; { - // Generate the common ovk for recovering t->z outputs. + // Generate the old, pre-UA accounts OVK for recovering t->z outputs. HDSeed seed = pwalletMain->GetHDSeedForRPC(); ovks.insert(ovkForShieldingFromTaddr(seed)); - auto legacyAcctUFVK = pwalletMain->GetUnifiedFullViewingKeyByAccount(ZCASH_LEGACY_ACCOUNT); - if (legacyAcctUFVK.has_value()) { - auto legacyAcctOVKs = legacyAcctUFVK.value().GetTransparentOVKsForShielding(); - if (legacyAcctOVKs.has_value()) { - ovks.insert(legacyAcctOVKs.value().first); - ovks.insert(legacyAcctOVKs.value().second); - } - } + // Generate the OVKs for shielding from the legacy UA account + auto legacyKey = pwalletMain->GetLegacyAccountKey().ToAccountPubKey(); + auto legacyAcctOVKs = legacyKey.GetOVKsForShielding(); + ovks.insert(legacyAcctOVKs.first); + ovks.insert(legacyAcctOVKs.second); } // Sapling spends @@ -4386,6 +4383,24 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) "Invalid from address, no payment source found for address."); } + auto selectorAccount = pwalletMain->FindAccountForSelector(ztxoSelectorOpt.value()); + std::visit(match { + [&](const libzcash::UnifiedAddress& ua) { + if (!selectorAccount.has_value() || selectorAccount.value() == ZCASH_LEGACY_ACCOUNT) { + throw JSONRPCError( + RPC_INVALID_ADDRESS_OR_KEY, + "Invalid from address, UA does not correspond to a known account."); + } + }, + [&](const auto& other) { + if (selectorAccount.has_value() && selectorAccount.value() != ZCASH_LEGACY_ACCOUNT) { + throw JSONRPCError( + RPC_INVALID_ADDRESS_OR_KEY, + "Invalid from address, bare address does not correspond the legacy account."); + } + } + }, decoded.value()); + return ztxoSelectorOpt.value(); } }(); diff --git a/src/wallet/test/rpc_wallet_tests.cpp b/src/wallet/test/rpc_wallet_tests.cpp index 1541baba0..07381622b 100644 --- a/src/wallet/test/rpc_wallet_tests.cpp +++ b/src/wallet/test/rpc_wallet_tests.cpp @@ -134,7 +134,7 @@ BOOST_AUTO_TEST_CASE(rpc_wallet) LOCK2(cs_main, pwalletMain->cs_wallet); - CPubKey demoPubkey = pwalletMain->GenerateNewKey(); + CPubKey demoPubkey = pwalletMain->GenerateNewKey(false); CTxDestination demoAddress(CTxDestination(demoPubkey.GetID())); UniValue retValue; string strPurpose = "receive"; @@ -143,7 +143,7 @@ BOOST_AUTO_TEST_CASE(rpc_wallet) pwalletMain->SetAddressBook(demoPubkey.GetID(), "", strPurpose); }); - CPubKey setaccountDemoPubkey = pwalletMain->GenerateNewKey(); + CPubKey setaccountDemoPubkey = pwalletMain->GenerateNewKey(false); CTxDestination setaccountDemoAddress(CTxDestination(setaccountDemoPubkey.GetID())); /********************************* @@ -1245,19 +1245,6 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) BOOST_CHECK( msg.find("Insufficient funds") != string::npos); } - // minconf cannot be zero when sending from zaddr - { - TransactionBuilder builder(consensusParams, nHeight + 1, pwalletMain); - try { - auto selector = pwalletMain->ToZTXOSelector(zaddr1, true).value(); - std::vector recipients = {SendManyRecipient(taddr1, 100*COIN, "DEADBEEF")}; - std::shared_ptr operation(new AsyncRPCOperation_sendmany(builder, selector, recipients, 0)); - BOOST_CHECK(false); // Fail test if an exception is not thrown - } catch (const UniValue& objError) { - BOOST_CHECK(find_error(objError, "Minconf cannot be zero when sending from a shielded address")); - } - } - // there are no unspent notes to spend { auto selector = pwalletMain->ToZTXOSelector(zaddr1, true).value(); @@ -1338,7 +1325,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_taddr_to_sapling) KeyIO keyIO(Params()); // add keys manually - auto taddr = pwalletMain->GenerateNewKey().GetID(); + auto taddr = pwalletMain->GenerateNewKey(false).GetID(); std::string taddr1 = keyIO.EncodeDestination(taddr); auto pa = pwalletMain->GenerateNewLegacySaplingZKey(); @@ -1402,13 +1389,19 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_taddr_to_sapling) tx.vShieldedOutput[0].cmu, tx.vShieldedOutput[0].ephemeralKey)); - // We should be able to decrypt the outCiphertext with the ovk - // generated for transparent addresses - std::optional seed = pwalletMain->GetMnemonicSeed(); - BOOST_ASSERT(seed.has_value()); + auto accountKey = pwalletMain->GetLegacyAccountKey().ToAccountPubKey(); + auto ovks = accountKey.GetOVKsForShielding(); + // We should not be able to decrypt with the internal change OVK for shielding + BOOST_CHECK(!AttemptSaplingOutDecryption( + tx.vShieldedOutput[0].outCiphertext, + ovks.first, + tx.vShieldedOutput[0].cv, + tx.vShieldedOutput[0].cmu, + tx.vShieldedOutput[0].ephemeralKey)); + // We should be able to decrypt with the external OVK for shielding BOOST_CHECK(AttemptSaplingOutDecryption( tx.vShieldedOutput[0].outCiphertext, - ovkForShieldingFromTaddr(seed.value()), + ovks.second, tx.vShieldedOutput[0].cv, tx.vShieldedOutput[0].cmu, tx.vShieldedOutput[0].ephemeralKey)); @@ -1994,7 +1987,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_mergetoaddress_internals) void TestWTxStatus(const Consensus::Params consensusParams, const int delta) { auto AddTrx = [&consensusParams]() { - auto taddr = pwalletMain->GenerateNewKey().GetID(); + auto taddr = pwalletMain->GenerateNewKey(false).GetID(); CMutableTransaction mtx = CreateNewContextualCMutableTransaction(consensusParams, 1); CScript scriptPubKey = CScript() << OP_DUP << OP_HASH160 << ToByteVector(taddr) << OP_EQUALVERIFY << OP_CHECKSIG; mtx.vout.push_back(CTxOut(5 * COIN, scriptPubKey)); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b56022f72..8abbd3ad0 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -256,9 +256,10 @@ bool CWallet::AddSproutZKey(const libzcash::SproutSpendingKey &key) return true; } -CPubKey CWallet::GenerateNewKey() +CPubKey CWallet::GenerateNewKey(bool isChangeKey) { AssertLockHeld(cs_wallet); // mapKeyMetadata + bool external = !isChangeKey; if (!mnemonicHDChain.has_value()) { throw std::runtime_error( @@ -270,13 +271,16 @@ CPubKey CWallet::GenerateNewKey() std::optional pubkey = std::nullopt; do { auto index = hdChain.GetLegacyTKeyCounter(); - auto key = accountKey.DeriveExternalSpendingKey(index); + auto key = external ? + accountKey.DeriveExternalSpendingKey(index) : + accountKey.DeriveInternalSpendingKey(index); + hdChain.IncrementLegacyTKeyCounter(); if (key.has_value()) { - auto keyPath = transparent::AccountKey::KeyPath(BIP44CoinType(), ZCASH_LEGACY_ACCOUNT, true, index); pubkey = AddTransparentSecretKey( hdChain.GetSeedFingerprint(), - std::make_pair(key.value(), keyPath) + key.value(), + transparent::AccountKey::KeyPath(BIP44CoinType(), ZCASH_LEGACY_ACCOUNT, external, index) ); } // if we did not successfully generate a key, try again. @@ -292,15 +296,15 @@ CPubKey CWallet::GenerateNewKey() CPubKey CWallet::AddTransparentSecretKey( const uint256& seedFingerprint, - const std::pair& extSecret) + const CKey& secret, + const HDKeyPath& keyPath) { - CKey secret = extSecret.first; CPubKey pubkey = secret.GetPubKey(); assert(secret.VerifyPubKey(pubkey)); // Create new metadata CKeyMetadata keyMeta(GetTime()); - keyMeta.hdKeypath = extSecret.second; + keyMeta.hdKeypath = keyPath, keyMeta.seedFp = seedFingerprint; mapKeyMetadata[pubkey.GetID()] = keyMeta; if (nTimeFirstKey == 0 || keyMeta.nCreateTime < nTimeFirstKey) @@ -481,7 +485,12 @@ std::optional // Set up the bidirectional maps between the account ID and the UFVK ID. auto metaKey = std::make_pair(skmeta.GetSeedFingerprint(), skmeta.GetAccountId()); - mapUnifiedAccountKeys.insert({metaKey, ufvkid}); + const auto [it, is_new_key] = mapUnifiedAccountKeys.insert({metaKey, skmeta.GetKeyID()}); + if (!is_new_key) { + // key was already present, so just return the USK. + return usk.value(); + } + // We set up the UFVKAddressMetadata with the correct account ID (so we identify // the UFVK as corresponding to this account) and empty receivers data (as we // haven't generated any addresses yet). We don't need to persist this directly, @@ -655,8 +664,36 @@ UAGenerationResult CWallet::GenerateUnifiedAddress( } assert(mapUfvkAddressMetadata[ufvkid].SetReceivers(diversifierIndex, receiverTypes)); - // Writing this data is handled by `CWalletDB::WriteUnifiedAddressMetadata` below. - assert(CCryptoKeyStore::AddTransparentReceiverForUnifiedAddress(ufvkid, diversifierIndex, address.value())); + if (hasTransparent) { + // We must construct the and add transparent spending key associated + // with the external and internal transparent child addresses to the + // transparent keystore. + auto usk = GenerateUnifiedSpendingKeyForAccount(accountId).value(); + auto accountKey = usk.GetTransparentKey(); + // this .value is known to be safe from the earlier check + auto childIndex = diversifierIndex.ToTransparentChildIndex().value(); + auto externalKey = accountKey.DeriveExternalSpendingKey(childIndex); + + if (!externalKey.has_value()) { + return AddressGenerationError::NoAddressForDiversifier; + } + + AddTransparentSecretKey( + mnemonicHDChain.value().GetSeedFingerprint(), + externalKey.value(), + transparent::AccountKey::KeyPath(BIP44CoinType(), accountId, true, childIndex) + ); + + // We do not add the change address for the transparent key, because + // we do not send transparent change when using unified accounts. + + // Writing this data is handled by `CWalletDB::WriteUnifiedAddressMetadata` below. + assert( + CCryptoKeyStore::AddTransparentReceiverForUnifiedAddress( + ufvkid, diversifierIndex, address.value() + ) + ); + } // Save the metadata for the generated address so that we can re-derive // it in the future. @@ -666,27 +703,6 @@ UAGenerationResult CWallet::GenerateUnifiedAddress( "CWallet::AddUnifiedAddress(): Writing unified address metadata failed"); } - if (hasTransparent) { - // Regenerate the secret key for the transparent address and add it to - // the wallet. - auto seed = GetMnemonicSeed().value(); - auto accountKey = transparent::AccountKey::ForAccount(seed, BIP44CoinType(), accountId).value(); - // this .value is known to be safe from the earlier check - auto childIndex = diversifierIndex.ToTransparentChildIndex().value(); - auto key = accountKey.DeriveExternalSpendingKey(childIndex).value(); - - AddTransparentSecretKey( - seed.Fingerprint(), - std::make_pair( - key, - transparent::AccountKey::KeyPath(BIP44CoinType(), accountId, true, childIndex) - ) - ); - - // We do not add the change address for the transparent key, because - // we do not send transparent change when using unified accounts. - } - return std::make_pair(address.value(), diversifierIndex); } else { return AddressGenerationError::NoSuchAccount; @@ -722,8 +738,12 @@ bool CWallet::LoadUnifiedFullViewingKey(const libzcash::UnifiedFullViewingKey &k // restore unified addresses that have been previously generated to the // keystore for (const auto &[j, receiverTypes] : metadata->second.GetKnownReceiverSetsByDiversifierIndex()) { - auto addr = zufvk.Address(j, receiverTypes).value(); - if (!CCryptoKeyStore::AddTransparentReceiverForUnifiedAddress(zufvk.GetKeyID(), j, addr)) { + auto addr = zufvk.Address(j, receiverTypes); + // failure to restore the generated address is an error + if (!addr.has_value()) return false; + + + if (!CCryptoKeyStore::AddTransparentReceiverForUnifiedAddress(zufvk.GetKeyID(), j, addr.value())) { return false; } } @@ -754,6 +774,7 @@ bool CWallet::LoadUnifiedAddressMetadata(const ZcashdUnifiedAddressMetadata &add // Regenerate the unified address and add it to the keystore. auto j = addrmeta.GetDiversifierIndex(); auto addr = ufvk.value().Address(j, addrmeta.GetReceiverTypes()).value(); + return CCryptoKeyStore::AddTransparentReceiverForUnifiedAddress(addrmeta.GetKeyID(), j, addr); } @@ -1573,16 +1594,26 @@ bool CWallet::SelectorMatchesAddress( std::optional CWallet::GenerateChangeAddressForAccount( libzcash::AccountId accountId, std::set changeOptions) { - auto ufvk = this->GetUnifiedFullViewingKeyByAccount(accountId); - if (ufvk.has_value()) { - for (libzcash::ChangeType t : changeOptions) { - if (t == libzcash::ChangeType::Transparent && accountId == ZCASH_LEGACY_ACCOUNT) { - return GenerateNewKey().GetID(); - } else { - return ufvk.value().GetChangeAddress(SaplingChangeRequest()); + AssertLockHeld(cs_wallet); + + // changeOptions is sorted in preference order, so return + // the first (and therefore most preferred) change address that + // we're able to generate. + for (libzcash::ChangeType t : changeOptions) { + if (t == libzcash::ChangeType::Transparent && accountId == ZCASH_LEGACY_ACCOUNT) { + return GenerateNewKey(true).GetID(); + } else { + auto ufvk = this->GetUnifiedFullViewingKeyByAccount(accountId); + if (ufvk.has_value()) { + // Default to Sapling shielded change (TODO ORCHARD: update this) + auto changeAddr = ufvk.value().GetChangeAddress(SaplingChangeRequest()); + if (changeAddr.has_value()) { + return changeAddr.value(); + } } } } + return std::nullopt; } @@ -4901,7 +4932,7 @@ bool CWallet::NewKeyPool() for (int i = 0; i < nKeys; i++) { int64_t nIndex = i+1; - walletdb.WritePool(nIndex, CKeyPool(GenerateNewKey())); + walletdb.WritePool(nIndex, CKeyPool(GenerateNewKey(false))); setKeyPool.insert(nIndex); } LogPrintf("CWallet::NewKeyPool wrote %d new keys\n", nKeys); @@ -4931,7 +4962,7 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) int64_t nEnd = 1; if (!setKeyPool.empty()) nEnd = *(--setKeyPool.end()) + 1; - if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey()))) + if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(false)))) throw runtime_error("TopUpKeyPool(): writing generated key failed"); setKeyPool.insert(nEnd); LogPrintf("keypool added key %d, size=%u\n", nEnd, setKeyPool.size()); @@ -4998,7 +5029,7 @@ std::optional CWallet::GetKeyFromPool() if (nIndex == -1) { if (IsLocked()) return std::nullopt; - return GenerateNewKey(); + return GenerateNewKey(false); } KeepKey(nIndex); return keypool.vchPubKey; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d638dcc29..c070a38ae 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1052,7 +1052,8 @@ private: /* Add a transparent secret key to the wallet. Internal use only. */ CPubKey AddTransparentSecretKey( const uint256& seedFingerprint, - const std::pair& extSecret); + const CKey& secret, + const HDKeyPath& keyPath); protected: bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx); @@ -1255,6 +1256,11 @@ public: */ std::optional FindAccountForSelector(const ZTXOSelector& paymentSource) const; + /** + * Generate a change address for the specified account. If a transparent change + * address is requested, this will generate a fresh diversified unified address, + * and return the associated transparent change address. + */ std::optional GenerateChangeAddressForAccount( libzcash::AccountId accountId, std::set changeOptions); @@ -1294,7 +1300,7 @@ public: * keystore implementation * Generate a new key */ - CPubKey GenerateNewKey(); + CPubKey GenerateNewKey(bool isChangeKey); //! Adds a key to the store, and saves it to disk. bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey); //! Adds a key to the store, without saving it to disk (used by LoadWallet)