From 235fde51931dbaef23efe50d47e252a4d34dddbb Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 23 Feb 2022 09:53:18 +0000 Subject: [PATCH] Add mappings from Orchard receivers to IVKs to the wallet We add the Orchard change address mapping on account creation, as we only ever generate a single change address internally. External Orchard receivers are added to the map when `z_getaddressforaccount` is called. This commit also fixes two bugs in the handling of Orchard internal IVKs. When adding a full viewing key to the Orchard (Rust) wallet, we need to derive both the internal and external IVKs (and map them to the external FVK so we effectively never expose the internal FVK). We were only deriving the external IVK, meaning that trying to add the Orchard change address to the wallet should have failed. However, in a separate bug the C++ internal IVK derivation method was calling the Rust external IVK derivation function, meaning that we were effectively using the external Orchard address at index 0 _as_ the change Orchard address. --- src/gtest/test_keystore.cpp | 29 +++++++++++++++++++++ src/rust/src/wallet.rs | 8 ++++-- src/wallet/gtest/test_wallet.cpp | 44 ++++++++++++++++++++++++++++++++ src/wallet/wallet.cpp | 42 +++++++++++++++++++++++++++++- src/wallet/wallet.h | 10 +++++++- src/zcash/address/orchard.cpp | 2 +- 6 files changed, 130 insertions(+), 5 deletions(-) diff --git a/src/gtest/test_keystore.cpp b/src/gtest/test_keystore.cpp index c99b504d6..5e9863cc1 100644 --- a/src/gtest/test_keystore.cpp +++ b/src/gtest/test_keystore.cpp @@ -578,6 +578,35 @@ TEST(KeystoreTests, StoreAndRetrieveUFVK) { EXPECT_FALSE(ufvkmeta.value().second.has_value()); } +TEST(KeystoreTests, StoreAndRetrieveUFVKByOrchard) { + SelectParams(CBaseChainParams::TESTNET); + CBasicKeyStore keyStore; + + auto seed = MnemonicSeed::Random(SLIP44_TESTNET_TYPE); + auto usk = ZcashdUnifiedSpendingKey::ForAccount(seed, SLIP44_TESTNET_TYPE, 0); + EXPECT_TRUE(usk.has_value()); + + auto ufvk = usk.value().ToFullViewingKey(); + auto zufvk = ZcashdUnifiedFullViewingKey::FromUnifiedFullViewingKey(Params(), ufvk); + auto ufvkid = zufvk.GetKeyID(); + EXPECT_FALSE(keyStore.GetUnifiedFullViewingKey(ufvkid).has_value()); + + EXPECT_TRUE(keyStore.AddUnifiedFullViewingKey(zufvk)); + EXPECT_EQ(keyStore.GetUnifiedFullViewingKey(ufvkid).value(), zufvk); + + auto addrPair = std::get>(zufvk.FindAddress(diversifier_index_t(0), {ReceiverType::Orchard})); + EXPECT_TRUE(addrPair.first.GetOrchardReceiver().has_value()); + auto orchardReceiver = addrPair.first.GetOrchardReceiver().value(); + + // We don't store Orchard addresses in CBasicKeyStore (the addr -> ivk + // mapping is stored in the Rust wallet), but we still detect this because + // we trial-decrypt diversifiers (which also means we learn the index). + auto ufvkmetaUnadded = keyStore.GetUFVKMetadataForReceiver(orchardReceiver); + EXPECT_TRUE(ufvkmetaUnadded.has_value()); + EXPECT_EQ(ufvkmetaUnadded.value().first, ufvkid); + EXPECT_EQ(ufvkmetaUnadded.value().second.value(), addrPair.second); +} + TEST(KeystoreTests, AddTransparentReceiverForUnifiedAddress) { SelectParams(CBaseChainParams::TESTNET); CBasicKeyStore keyStore; diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 7ebeb4a54..2ff15f336 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -73,8 +73,12 @@ impl KeyStore { } pub fn add_full_viewing_key(&mut self, fvk: FullViewingKey) { - let ivk = IncomingViewingKey::from(&fvk); - self.viewing_keys.insert(ivk, fvk); + // When we add a full viewing key, we need to add both the internal and external + // incoming viewing keys. + let external_ivk = IncomingViewingKey::from(&fvk); + let internal_ivk = IncomingViewingKey::from(&fvk.derive_internal()); + self.viewing_keys.insert(external_ivk, fvk.clone()); + self.viewing_keys.insert(internal_ivk, fvk); } pub fn add_spending_key(&mut self, sk: SpendingKey) { diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index dc0123cac..a745c5cef 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -44,6 +44,10 @@ class TestWallet : public CWallet { public: TestWallet(const CChainParams& params) : CWallet(params) { } + OrchardWallet& GetOrchardWallet() { + return orchardWallet; + } + bool EncryptKeys(CKeyingMaterial& vMasterKeyIn) { return CCryptoKeyStore::EncryptKeys(vMasterKeyIn); } @@ -2245,3 +2249,43 @@ TEST(WalletTests, GenerateUnifiedAddress) { // Revert to default RegtestDeactivateSapling(); } + +TEST(WalletTests, GenerateUnifiedSpendingKeyAddsOrchardAddresses) { + (void) RegtestActivateSapling(); + TestWallet wallet(Params()); + wallet.GenerateNewSeed(); + + // Create an account. + auto ufvkpair = wallet.GenerateNewUnifiedSpendingKey(); + auto ufvk = ufvkpair.first; + auto account = ufvkpair.second; + auto fvk = ufvk.GetOrchardKey(); + EXPECT_TRUE(fvk.has_value()); + + // At this point the Orchard wallet should contain the change address, but + // no other addresses. We detect this by trying to look up their IVKs. + auto changeAddr = fvk->ToInternalIncomingViewingKey().Address(libzcash::diversifier_index_t{0}); + auto internalIvk = wallet.GetOrchardWallet().GetIncomingViewingKeyForAddress(changeAddr); + EXPECT_TRUE(internalIvk.has_value()); + EXPECT_EQ(internalIvk.value(), fvk->ToInternalIncomingViewingKey()); + auto externalAddr = fvk->ToIncomingViewingKey().Address(libzcash::diversifier_index_t{0}); + auto externalIvk = wallet.GetOrchardWallet().GetIncomingViewingKeyForAddress(externalAddr); + EXPECT_FALSE(externalIvk.has_value()); + + // Generate an address. + auto uaResult = wallet.GenerateUnifiedAddress(account, {ReceiverType::Orchard}); + auto ua = std::get_if>(&uaResult); + EXPECT_NE(ua, nullptr); + + auto uaOrchardReceiver = ua->first.GetOrchardReceiver(); + EXPECT_TRUE(uaOrchardReceiver.has_value()); + EXPECT_EQ(uaOrchardReceiver.value(), externalAddr); + + // Now we can look up the external IVK. + externalIvk = wallet.GetOrchardWallet().GetIncomingViewingKeyForAddress(externalAddr); + EXPECT_TRUE(externalIvk.has_value()); + EXPECT_EQ(externalIvk.value(), fvk->ToIncomingViewingKey()); + + // Revert to default + RegtestDeactivateSapling(); +} diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5aca42d7e..631cfced0 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -268,6 +268,25 @@ bool CWallet::AddOrchardFullViewingKey(const libzcash::OrchardFullViewingKey &fv return true; // TODO ORCHARD: persist fvk } +// Add Orchard payment address -> incoming viewing key map entry +bool CWallet::AddOrchardRawAddress( + const libzcash::OrchardIncomingViewingKey &ivk, + const libzcash::OrchardRawAddress &addr) +{ + AssertLockHeld(cs_wallet); // orchardWallet + if (!orchardWallet.AddRawAddress(addr, ivk)) { + // We should never add an Orchard raw address for which we don't know + // the corresponding FVK. + return false; + }; + + if (!fFileBacked) { + return true; + } + + return true; // TODO ORCHARD: ensure mapping will be recreated on wallet load +} + // Loads a payment address -> incoming viewing key map entry // to the in-memory wallet's keystore. bool CWallet::LoadOrchardRawAddress( @@ -577,7 +596,17 @@ std::optional }; // Add Orchard spending key to the wallet - orchardWallet.AddSpendingKey(usk.value().GetOrchardKey()); + auto orchardSk = usk.value().GetOrchardKey(); + orchardWallet.AddSpendingKey(orchardSk); + + // Associate the Orchard default change address with its IVK. We do this + // here because there is only ever a single Orchard change receiver, and + // it is never exposed to the user. External Orchard receivers are added + // when the user calls z_getaddressforaccount. + auto orchardInternalFvk = orchardSk.ToFullViewingKey().ToInternalIncomingViewingKey(); + if (!AddOrchardRawAddress(orchardInternalFvk, orchardInternalFvk.Address(0))) { + throw std::runtime_error("CWallet::GenerateUnifiedSpendingKeyForAccount(): Failed to add Sapling change address to the wallet."); + }; auto zufvk = ZcashdUnifiedFullViewingKey::FromUnifiedFullViewingKey(Params(), ufvk); if (!CCryptoKeyStore::AddUnifiedFullViewingKey(zufvk)) { @@ -767,6 +796,17 @@ WalletUAGenerationResult CWallet::GenerateUnifiedAddress( AddSaplingPaymentAddress(dfvk.value().ToIncomingViewingKey(), saplingAddress.value()); } + // If the address has an Orchard component, add an association between + // that address and the Orchard IVK corresponding to the ufvk + auto hasOrchard = receiverTypes.find(ReceiverType::Orchard) != receiverTypes.end(); + if (hasOrchard) { + auto fvk = ufvk.value().GetOrchardKey(); + auto orchardReceiver = address.first.GetOrchardReceiver(); + assert (fvk.has_value() && orchardReceiver.has_value()); + + AddOrchardRawAddress(fvk.value().ToIncomingViewingKey(), orchardReceiver.value()); + } + // Save the metadata for the generated address so that we can re-derive // it in the future. ZcashdUnifiedAddressMetadata addrmeta(ufvkid, address.second, receiverTypes); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index bbe1279e2..7bbdef52d 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1463,7 +1463,15 @@ public: bool AddOrchardZKey(const libzcash::OrchardSpendingKey &sk); bool AddOrchardFullViewingKey(const libzcash::OrchardFullViewingKey &fvk); /** - * Adds an address/ivk mapping to the in-memory wallet. Returns `true` + * Adds an address/ivk mapping to the in-memory wallet. Returns `false` if + * the mapping could not be persisted, or the IVK does not correspond to an + * FVK known by the wallet. + */ + bool AddOrchardRawAddress( + const libzcash::OrchardIncomingViewingKey &ivk, + const libzcash::OrchardRawAddress &addr); + /** + * Loads an address/ivk mapping to the in-memory wallet. Returns `true` * if the provided IVK corresponds to an FVK known by the wallet. */ bool LoadOrchardRawAddress( diff --git a/src/zcash/address/orchard.cpp b/src/zcash/address/orchard.cpp index bbcf39143..a1271a4b0 100644 --- a/src/zcash/address/orchard.cpp +++ b/src/zcash/address/orchard.cpp @@ -24,7 +24,7 @@ OrchardIncomingViewingKey OrchardFullViewingKey::ToIncomingViewingKey() const { } OrchardIncomingViewingKey OrchardFullViewingKey::ToInternalIncomingViewingKey() const { - return OrchardIncomingViewingKey(orchard_full_viewing_key_to_incoming_viewing_key(inner.get())); + return OrchardIncomingViewingKey(orchard_full_viewing_key_to_internal_incoming_viewing_key(inner.get())); } OrchardSpendingKey OrchardSpendingKey::ForAccount(