From 45c4568a7e92347703747b2ad3f74cb387aa47c9 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Wed, 8 Mar 2023 18:14:08 -0700 Subject: [PATCH] Simplify diversifier_index_t handling - Remove `std::optional` from a number of uses, - simplify `GetUFVKMetadataForAddress` to `GetUFVKIdForAddress`, and - add a new `GetUFVKMetadataForAddress` as a wrapper around `GetUFVKMetadataForReceiver`. --- src/gtest/test_keystore.cpp | 6 +++--- src/keystore.cpp | 34 ++++++++++++------------------- src/keystore.h | 22 ++++++++++---------- src/wallet/wallet.cpp | 40 ++++++++++++------------------------- 4 files changed, 41 insertions(+), 61 deletions(-) diff --git a/src/gtest/test_keystore.cpp b/src/gtest/test_keystore.cpp index 4d9cfbee3..8af6b39d3 100644 --- a/src/gtest/test_keystore.cpp +++ b/src/gtest/test_keystore.cpp @@ -564,7 +564,7 @@ TEST(KeystoreTests, StoreAndRetrieveUFVK) { auto ufvkmetaUnadded = keyStore.GetUFVKMetadataForReceiver(saplingReceiver); EXPECT_TRUE(ufvkmetaUnadded.has_value()); EXPECT_EQ(ufvkmetaUnadded.value().GetUFVKId(), ufvkid); - EXPECT_EQ(ufvkmetaUnadded.value().GetDiversifierIndex().value(), addrPair.second); + EXPECT_EQ(ufvkmetaUnadded.value().GetDiversifierIndex(), addrPair.second); // Adding the Sapling addr -> ivk map entry causes us to find the same UFVK, // and since we trial-decrypt with both external and internal IVKs to @@ -575,7 +575,7 @@ TEST(KeystoreTests, StoreAndRetrieveUFVK) { auto ufvkmeta = keyStore.GetUFVKMetadataForReceiver(saplingReceiver); EXPECT_TRUE(ufvkmeta.has_value()); EXPECT_EQ(ufvkmeta.value().GetUFVKId(), ufvkid); - EXPECT_TRUE(ufvkmeta.value().GetDiversifierIndex().has_value()); + EXPECT_EQ(ufvkmeta.value().GetDiversifierIndex(), addrPair.second); } TEST(KeystoreTests, StoreAndRetrieveUFVKByOrchard) { @@ -604,7 +604,7 @@ TEST(KeystoreTests, StoreAndRetrieveUFVKByOrchard) { auto ufvkmetaUnadded = keyStore.GetUFVKMetadataForReceiver(orchardReceiver); EXPECT_TRUE(ufvkmetaUnadded.has_value()); EXPECT_EQ(ufvkmetaUnadded.value().GetUFVKId(), ufvkid); - EXPECT_EQ(ufvkmetaUnadded.value().GetDiversifierIndex().value(), addrPair.second); + EXPECT_EQ(ufvkmetaUnadded.value().GetDiversifierIndex(), addrPair.second); } TEST(KeystoreTests, AddTransparentReceiverForUnifiedAddress) { diff --git a/src/keystore.cpp b/src/keystore.cpp index b7fe70c52..be452294a 100644 --- a/src/keystore.cpp +++ b/src/keystore.cpp @@ -15,6 +15,16 @@ bool CKeyStore::AddKey(const CKey &key) { return AddKeyPubKey(key, key.GetPubKey()); } +std::optional CKeyStore::GetUFVKMetadataForAddress( + const CTxDestination& address) const +{ + auto self = this; + return std::visit(match { + [](const CNoDestination&) -> std::optional { return std::nullopt; }, + [&](const auto& addr) { return self->GetUFVKMetadataForReceiver(addr); } + }, address); +} + bool CBasicKeyStore::GetPubKey(const CKeyID &address, CPubKey &vchPubKeyOut) const { CKey key; @@ -390,12 +400,10 @@ CBasicKeyStore::GetUFVKMetadataForReceiver(const libzcash::Receiver& receiver) c return std::visit(FindUFVKId(*this), receiver); } -std::optional -CBasicKeyStore::GetUFVKMetadataForAddress(const libzcash::UnifiedAddress& addr) const +std::optional +CBasicKeyStore::GetUFVKIdForAddress(const libzcash::UnifiedAddress& addr) const { std::optional ufvkId; - std::optional j; - bool jConflict = false; for (const auto& receiver : addr) { auto rmeta = GetUFVKMetadataForReceiver(receiver); if (rmeta.has_value()) { @@ -408,29 +416,13 @@ CBasicKeyStore::GetUFVKMetadataForAddress(const libzcash::UnifiedAddress& addr) if (rmeta.value().GetUFVKId() != ufvkId.value()) { return std::nullopt; } - - if (rmeta.value().GetDiversifierIndex().has_value()) { - if (j.has_value()) { - if (rmeta.value().GetDiversifierIndex().value() != j.value()) { - jConflict = true; - j = std::nullopt; - } - } else if (!jConflict) { - j = rmeta.value().GetDiversifierIndex().value(); - } - } } else { ufvkId = rmeta.value().GetUFVKId(); - j = rmeta.value().GetDiversifierIndex(); } } } - if (ufvkId.has_value()) { - return AddressUFVKMetadata(ufvkId.value(), j, true); - } else { - return std::nullopt; - } + return ufvkId; } std::optional CBasicKeyStore::GetUFVKIdForViewingKey(const libzcash::ViewingKey& vk) const diff --git a/src/keystore.h b/src/keystore.h index cf2321703..62bf3c0f6 100644 --- a/src/keystore.h +++ b/src/keystore.h @@ -22,14 +22,14 @@ class AddressUFVKMetadata { private: libzcash::UFVKId ufvkId; - std::optional j; + libzcash::diversifier_index_t j; bool externalAddress; public: - AddressUFVKMetadata(libzcash::UFVKId ufvkId, std::optional j, bool externalAddress) + AddressUFVKMetadata(libzcash::UFVKId ufvkId, libzcash::diversifier_index_t j, bool externalAddress) : ufvkId(ufvkId), j(j), externalAddress(externalAddress) {} libzcash::UFVKId GetUFVKId() const { return ufvkId; } - std::optional GetDiversifierIndex() const { return j; } + libzcash::diversifier_index_t GetDiversifierIndex() const { return j; } bool IsExternalAddress() const { return externalAddress; } }; @@ -145,12 +145,14 @@ public: virtual std::optional GetUFVKMetadataForReceiver( const libzcash::Receiver& receiver) const = 0; + std::optional GetUFVKMetadataForAddress( + const CTxDestination& address) const; + /** * If all the receivers of the specified address correspond to a single - * UFVK, return that key's metadata. If all the receivers correspond to - * the same diversifier index, that diversifier index is also returned. + * UFVK, return that key's metadata. */ - virtual std::optional GetUFVKMetadataForAddress( + virtual std::optional GetUFVKIdForAddress( const libzcash::UnifiedAddress& addr) const = 0; virtual std::optional GetUFVKIdForViewingKey( @@ -405,14 +407,14 @@ public: } } - virtual std::optional GetUFVKMetadataForAddress( + virtual std::optional GetUFVKIdForAddress( const libzcash::UnifiedAddress& addr) const; std::optional GetUFVKForAddress( const libzcash::UnifiedAddress& addr) const { - auto ufvkMeta = GetUFVKMetadataForAddress(addr); - if (ufvkMeta.has_value()) { - return GetUnifiedFullViewingKey(ufvkMeta.value().GetUFVKId()); + auto ufvkId = GetUFVKIdForAddress(addr); + if (ufvkId.has_value()) { + return GetUnifiedFullViewingKey(ufvkId.value()); } else { return std::nullopt; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c289b0883..a0f84da97 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1913,14 +1913,14 @@ std::optional CWallet::ZTXOSelectorForAddress( } }, [&](const libzcash::UnifiedAddress& ua) { - auto ufvkMeta = GetUFVKMetadataForAddress(ua); - if (ufvkMeta.has_value()) { + auto ufvkId = GetUFVKIdForAddress(ua); + if (ufvkId.has_value()) { // TODO: at present, the `false` value for the `requireSpendingKey` argument // is not respected for unified addresses, because we have no notion of // an account for which we do not control the spending key. An alternate // approach would be to use the UFVK directly in the case that we cannot // determine a local account. - auto accountId = this->GetUnifiedAccountId(ufvkMeta.value().GetUFVKId()); + auto accountId = this->GetUnifiedAccountId(ufvkId.value()); if (accountId.has_value()) { if (allowAddressLinkability) { pattern = AccountZTXOPattern(accountId.value(), ua.GetKnownReceiverTypes()); @@ -2013,9 +2013,9 @@ std::optional CWallet::FindAccountForSelector(const ZTXOSel } }, [&](const libzcash::UnifiedAddress& addr) { - auto meta = GetUFVKMetadataForAddress(addr); - if (meta.has_value()) { - result = self->GetUnifiedAccountId(meta.value().GetUFVKId()); + auto ufvkId = GetUFVKIdForAddress(addr); + if (ufvkId.has_value()) { + result = self->GetUnifiedAccountId(ufvkId.value()); } }, [&](const libzcash::UnifiedFullViewingKey& vk) { @@ -2074,20 +2074,12 @@ bool CWallet::SelectorMatchesAddress( return false; }, [&](const libzcash::UnifiedFullViewingKey& ufvk) { - std::optional meta; - std::visit(match { - [&](const CNoDestination& none) { meta = std::nullopt; }, - [&](const auto& addr) { meta = self->GetUFVKMetadataForReceiver(addr); } - }, address); + auto meta = self->GetUFVKMetadataForAddress(address); return (meta.has_value() && meta.value().GetUFVKId() == ufvk.GetKeyID(Params())); }, [&](const AccountZTXOPattern& acct) { if (acct.IncludesP2PKH() || acct.IncludesP2SH()) { - std::optional meta; - std::visit(match { - [&](const CNoDestination& none) { meta = std::nullopt; }, - [&](const auto& addr) { meta = self->GetUFVKMetadataForReceiver(addr); } - }, address); + auto meta = self->GetUFVKMetadataForAddress(address); if (meta.has_value()) { // use the coin if the account id corresponding to the UFVK is // the payment source account. @@ -7422,13 +7414,13 @@ PaymentAddressSource GetSourceForPaymentAddress::operator()(const libzcash::Sapl PaymentAddressSource GetSourceForPaymentAddress::operator()(const libzcash::UnifiedAddress &uaddr) const { auto hdChain = m_wallet->GetMnemonicHDChain(); - auto ufvkMeta = m_wallet->GetUFVKMetadataForAddress(uaddr); - if (ufvkMeta.has_value()) { + auto ufvkId = m_wallet->GetUFVKIdForAddress(uaddr); + if (ufvkId.has_value()) { // Look through the UFVKs that we have generated, and confirm that the - // seed fingerprint for the key we find for the ufvkMeta corresponds to + // seed fingerprint for the key we find for the ufvkId corresponds to // the wallet's mnemonic seed. for (const auto& [k, v] : m_wallet->mapUnifiedAccountKeys) { - if (v == ufvkMeta.value().GetUFVKId() && hdChain.has_value() && k.first == hdChain.value().GetSeedFingerprint()) { + if (v == ufvkId.value() && hdChain.has_value() && k.first == hdChain.value().GetSeedFingerprint()) { return PaymentAddressSource::MnemonicHDSeed; } } @@ -7614,9 +7606,6 @@ std::optional UFVKForReceiver::operator() auto ufvkMeta = wallet.GetUFVKMetadataForReceiver(keyId); if (ufvkMeta.has_value()) { auto ufvkid = ufvkMeta.value().GetUFVKId(); - // transparent address UFVK metadata is always accompanied by the child - // index at which the address was produced - assert(ufvkMeta.value().GetDiversifierIndex().has_value()); auto ufvk = wallet.GetUnifiedFullViewingKey(ufvkid); assert(ufvk.has_value() && ufvk.value().GetTransparentKey().has_value()); return ufvk.value(); @@ -7696,10 +7685,7 @@ std::optional UnifiedAddressForReceiver::operator()(co auto ufvkMeta = wallet.GetUFVKMetadataForReceiver(keyId); if (ufvkMeta.has_value()) { auto ufvkid = ufvkMeta.value().GetUFVKId(); - // transparent address UFVK metadata is always accompanied by the child - // index at which the address was produced - assert(ufvkMeta.value().GetDiversifierIndex().has_value()); - diversifier_index_t j = ufvkMeta.value().GetDiversifierIndex().value(); + diversifier_index_t j = ufvkMeta.value().GetDiversifierIndex(); auto ufvk = wallet.GetUnifiedFullViewingKey(ufvkid); if (!(ufvk.has_value() && ufvk.value().GetTransparentKey().has_value())) { throw std::runtime_error("CWallet::UnifiedAddressForReceiver(): UFVK has no P2PKH key part.");