From 235fde51931dbaef23efe50d47e252a4d34dddbb Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 23 Feb 2022 09:53:18 +0000 Subject: [PATCH 1/2] 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( From b0769e3f1d3a1050c8015a970187f1aac713790e Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 23 Feb 2022 10:04:34 +0000 Subject: [PATCH 2/2] Add Orchard to default UA receiver types We also add Orchard-specific information to several RPCs in order for tests to pass: - `z_listunifiedreceivers` - `z_getbalanceforaccount` - `z_getbalanceforviewingkey` --- qa/rpc-tests/wallet_accounts.py | 30 +++++++++++++++++++++++++---- qa/rpc-tests/wallet_listreceived.py | 3 ++- src/wallet/rpcwallet.cpp | 25 ++++++++++++++++++++++-- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 4 ++++ src/zcash/address/unified.cpp | 2 +- 6 files changed, 57 insertions(+), 9 deletions(-) diff --git a/qa/rpc-tests/wallet_accounts.py b/qa/rpc-tests/wallet_accounts.py index 15f097d78..34a1e7fd5 100755 --- a/qa/rpc-tests/wallet_accounts.py +++ b/qa/rpc-tests/wallet_accounts.py @@ -55,7 +55,7 @@ class WalletAccountsTest(BitcoinTestFramework): # Generate the first address for account 0. addr0 = self.nodes[0].z_getaddressforaccount(0) assert_equal(addr0['account'], 0) - assert_equal(set(addr0['pools']), set(['transparent', 'sapling'])) + assert_equal(set(addr0['pools']), set(['transparent', 'sapling', 'orchard'])) ua0 = addr0['unifiedaddress'] # We pick mnemonic phrases to ensure that we can always generate the default @@ -70,16 +70,38 @@ class WalletAccountsTest(BitcoinTestFramework): 'no address at diversifier index 0', self.nodes[0].z_getaddressforaccount, 0, [], 0) + # The second address for account 0 is different to the first address. + addr0_2 = self.nodes[0].z_getaddressforaccount(0) + assert_equal(addr0_2['account'], 0) + assert_equal(set(addr0_2['pools']), set(['transparent', 'sapling', 'orchard'])) + ua0_2 = addr0_2['unifiedaddress'] + assert(ua0 != ua0_2) + + # We can generate a fully-shielded address. + addr0_3 = self.nodes[0].z_getaddressforaccount(0, ['sapling', 'orchard']) + assert_equal(addr0_3['account'], 0) + assert_equal(set(addr0_3['pools']), set(['sapling', 'orchard'])) + ua0_3 = addr0_3['unifiedaddress'] + + # We can generate an address without a Sapling receiver. + addr0_4 = self.nodes[0].z_getaddressforaccount(0, ['transparent', 'orchard']) + assert_equal(addr0_4['account'], 0) + assert_equal(set(addr0_4['pools']), set(['transparent', 'orchard'])) + ua0_4 = addr0_4['unifiedaddress'] + # The first address for account 1 is different to account 0. addr1 = self.nodes[0].z_getaddressforaccount(1) assert_equal(addr1['account'], 1) - assert_equal(set(addr1['pools']), set(['transparent', 'sapling'])) + assert_equal(set(addr1['pools']), set(['transparent', 'sapling', 'orchard'])) ua1 = addr1['unifiedaddress'] assert(ua0 != ua1) # The UA contains the expected receiver kinds. - self.check_receiver_types(ua0, ['transparent', 'sapling']) - self.check_receiver_types(ua1, ['transparent', 'sapling']) + self.check_receiver_types(ua0, ['transparent', 'sapling', 'orchard']) + self.check_receiver_types(ua0_2, ['transparent', 'sapling', 'orchard']) + self.check_receiver_types(ua0_3, [ 'sapling', 'orchard']) + self.check_receiver_types(ua0_4, ['transparent', 'orchard']) + self.check_receiver_types(ua1, ['transparent', 'sapling', 'orchard']) # The balances of the accounts are all zero. self.check_balance(0, 0, ua0, {}) diff --git a/qa/rpc-tests/wallet_listreceived.py b/qa/rpc-tests/wallet_listreceived.py index fd67ebb76..ec809eec3 100755 --- a/qa/rpc-tests/wallet_listreceived.py +++ b/qa/rpc-tests/wallet_listreceived.py @@ -381,9 +381,10 @@ class ListReceivedTest (BitcoinTestFramework): r = node.z_getaddressforaccount(account) unified_addr = r['unifiedaddress'] receivers = node.z_listunifiedreceivers(unified_addr) - assert_equal(len(receivers), 2) + assert_equal(len(receivers), 3) assert 'transparent' in receivers assert 'sapling' in receivers + assert 'orchard' in receivers # Wallet contains no notes r = node.z_listreceivedbyaddress(unified_addr, 0) assert_equal(len(r), 0, "unified_addr should have received zero notes") diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index aa99bb0cb..fd8cc05df 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3119,14 +3119,16 @@ UniValue z_getaddressforaccount(const UniValue& params, bool fHelp) receivers.insert(ReceiverType::P2PKH); } else if (p == "sapling") { receivers.insert(ReceiverType::Sapling); + } else if (p == "orchard") { + receivers.insert(ReceiverType::Orchard); } else { - throw JSONRPCError(RPC_INVALID_PARAMETER, "pool arguments must be \"transparent\", or \"sapling\""); + throw JSONRPCError(RPC_INVALID_PARAMETER, "pool arguments must be \"transparent\", \"sapling\", or \"orchard\""); } } } if (receivers.empty()) { // Default is the best and second-best shielded pools, and the transparent pool. - receivers = {ReceiverType::P2PKH, ReceiverType::Sapling}; + receivers = {ReceiverType::P2PKH, ReceiverType::Sapling, ReceiverType::Orchard}; } std::optional j = std::nullopt; @@ -3220,6 +3222,9 @@ UniValue z_getaddressforaccount(const UniValue& params, bool fHelp) case ReceiverType::Sapling: pools.push_back("sapling"); break; + case ReceiverType::Orchard: + pools.push_back("orchard"); + break; default: // Unreachable assert(false); @@ -3331,6 +3336,12 @@ UniValue z_listunifiedreceivers(const UniValue& params, bool fHelp) UniValue result(UniValue::VOBJ); for (const auto& receiver : ua) { std::visit(match { + [&](const libzcash::OrchardRawAddress& addr) { + // Create a single-receiver UA that just contains this Orchard receiver. + UnifiedAddress singleReceiver; + singleReceiver.AddReceiver(addr); + result.pushKV("orchard", keyIO.EncodePaymentAddress(singleReceiver)); + }, [&](const libzcash::SaplingPaymentAddress& addr) { result.pushKV("sapling", keyIO.EncodePaymentAddress(addr)); }, @@ -3762,6 +3773,7 @@ UniValue z_getbalanceforviewingkey(const UniValue& params, bool fHelp) CAmount transparentBalance = 0; CAmount sproutBalance = 0; CAmount saplingBalance = 0; + CAmount orchardBalance = 0; for (const auto& t : spendableInputs.utxos) { transparentBalance += t.Value(); } @@ -3771,6 +3783,9 @@ UniValue z_getbalanceforviewingkey(const UniValue& params, bool fHelp) for (const auto& t : spendableInputs.saplingNoteEntries) { saplingBalance += t.note.value(); } + for (const auto& t : spendableInputs.orchardNoteMetadata) { + orchardBalance += t.GetNoteValue(); + } UniValue pools(UniValue::VOBJ); auto renderBalance = [&](std::string poolName, CAmount balance) { @@ -3783,6 +3798,7 @@ UniValue z_getbalanceforviewingkey(const UniValue& params, bool fHelp) renderBalance("transparent", transparentBalance); renderBalance("sprout", sproutBalance); renderBalance("sapling", saplingBalance); + renderBalance("orchard", orchardBalance); UniValue result(UniValue::VOBJ); result.pushKV("pools", pools); @@ -3863,12 +3879,16 @@ UniValue z_getbalanceforaccount(const UniValue& params, bool fHelp) CAmount transparentBalance = 0; CAmount saplingBalance = 0; + CAmount orchardBalance = 0; for (const auto& t : spendableInputs.utxos) { transparentBalance += t.Value(); } for (const auto& t : spendableInputs.saplingNoteEntries) { saplingBalance += t.note.value(); } + for (const auto& t : spendableInputs.orchardNoteMetadata) { + orchardBalance += t.GetNoteValue(); + } UniValue pools(UniValue::VOBJ); auto renderBalance = [&](std::string poolName, CAmount balance) { @@ -3880,6 +3900,7 @@ UniValue z_getbalanceforaccount(const UniValue& params, bool fHelp) }; renderBalance("transparent", transparentBalance); renderBalance("sapling", saplingBalance); + renderBalance("orchard", orchardBalance); UniValue result(UniValue::VOBJ); result.pushKV("pools", pools); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 631cfced0..e03e57514 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -605,7 +605,7 @@ std::optional // 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."); + throw std::runtime_error("CWallet::GenerateUnifiedSpendingKeyForAccount(): Failed to add Orchard change address to the wallet."); }; auto zufvk = ZcashdUnifiedFullViewingKey::FromUnifiedFullViewingKey(Params(), ufvk); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 7bbdef52d..7a25a2947 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -794,6 +794,7 @@ public: std::vector utxos; std::vector sproutNoteEntries; std::vector saplingNoteEntries; + std::vector orchardNoteMetadata; /** * Selectively discard notes that are not required to obtain the desired @@ -816,6 +817,9 @@ public: for (const auto& t : saplingNoteEntries) { result += t.note.value(); } + for (const auto& t : orchardNoteMetadata) { + result += t.GetNoteValue(); + } return result; } diff --git a/src/zcash/address/unified.cpp b/src/zcash/address/unified.cpp index 93dd1b34f..9fe8c131a 100644 --- a/src/zcash/address/unified.cpp +++ b/src/zcash/address/unified.cpp @@ -150,7 +150,7 @@ UnifiedAddressGenerationResult ZcashdUnifiedFullViewingKey::FindAddress( UnifiedAddressGenerationResult ZcashdUnifiedFullViewingKey::FindAddress( const diversifier_index_t& j) const { - return FindAddress(j, {ReceiverType::P2PKH, ReceiverType::Sapling}); + return FindAddress(j, {ReceiverType::P2PKH, ReceiverType::Sapling, ReceiverType::Orchard}); } std::optional ZcashdUnifiedFullViewingKey::GetChangeAddress(const ChangeRequest& req) const {