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

@@ -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) {

src/keystore.cpp

@@ -15,6 +15,16 @@ bool CKeyStore::AddKey(const CKey &key) {
     return AddKeyPubKey(key, key.GetPubKey());
 }
 
+std::optional<AddressUFVKMetadata> CKeyStore::GetUFVKMetadataForAddress(
+        const CTxDestination& address) const
+{
+    auto self = this;
+    return std::visit(match {
+            [](const CNoDestination&) -> std::optional<AddressUFVKMetadata> { 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<AddressUFVKMetadata>
+std::optional<libzcash::UFVKId>
-CBasicKeyStore::GetUFVKMetadataForAddress(const libzcash::UnifiedAddress& addr) const
+CBasicKeyStore::GetUFVKIdForAddress(const libzcash::UnifiedAddress& addr) const
 {
     std::optional<libzcash::UFVKId> ufvkId;
-    std::optional<libzcash::diversifier_index_t> 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 ufvkId;
-        return AddressUFVKMetadata(ufvkId.value(), j, true);
-    } else {
-        return std::nullopt;
-    }
 }
 
 std::optional<libzcash::UFVKId> CBasicKeyStore::GetUFVKIdForViewingKey(const libzcash::ViewingKey& vk) const

src/keystore.h

@@ -22,14 +22,14 @@
 class AddressUFVKMetadata {
 private:
     libzcash::UFVKId ufvkId;
-    std::optional<libzcash::diversifier_index_t> j;
+    libzcash::diversifier_index_t j;
     bool externalAddress;
 public:
-    AddressUFVKMetadata(libzcash::UFVKId ufvkId, std::optional<libzcash::diversifier_index_t> 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<libzcash::diversifier_index_t> 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<AddressUFVKMetadata> GetUFVKMetadataForReceiver(
             const libzcash::Receiver& receiver) const = 0;
 
+    std::optional<AddressUFVKMetadata> 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
+     * UFVK, return that key's metadata.
-     * the same diversifier index, that diversifier index is also returned.
      */
-    virtual std::optional<AddressUFVKMetadata> GetUFVKMetadataForAddress(
+    virtual std::optional<libzcash::UFVKId> GetUFVKIdForAddress(
             const libzcash::UnifiedAddress& addr) const = 0;
 
     virtual std::optional<libzcash::UFVKId> GetUFVKIdForViewingKey(
@@ -405,14 +407,14 @@ public:
         }
     }
 
-    virtual std::optional<AddressUFVKMetadata> GetUFVKMetadataForAddress(
+    virtual std::optional<libzcash::UFVKId> GetUFVKIdForAddress(
             const libzcash::UnifiedAddress& addr) const;
 
     std::optional<libzcash::ZcashdUnifiedFullViewingKey> GetUFVKForAddress(
             const libzcash::UnifiedAddress& addr) const {
-        auto ufvkMeta = GetUFVKMetadataForAddress(addr);
+        auto ufvkId = GetUFVKIdForAddress(addr);
-        if (ufvkMeta.has_value()) {
+        if (ufvkId.has_value()) {
-            return GetUnifiedFullViewingKey(ufvkMeta.value().GetUFVKId());
+            return GetUnifiedFullViewingKey(ufvkId.value());
         } else {
             return std::nullopt;
         }

src/wallet/wallet.cpp

@@ -1913,14 +1913,14 @@ std::optional<ZTXOSelector> CWallet::ZTXOSelectorForAddress(
             }
         },
         [&](const libzcash::UnifiedAddress& ua) {
-            auto ufvkMeta = GetUFVKMetadataForAddress(ua);
+            auto ufvkId = GetUFVKIdForAddress(ua);
-            if (ufvkMeta.has_value()) {
+            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<libzcash::AccountId> CWallet::FindAccountForSelector(const ZTXOSel
             }
         },
         [&](const libzcash::UnifiedAddress& addr) {
-            auto meta = GetUFVKMetadataForAddress(addr);
+            auto ufvkId = GetUFVKIdForAddress(addr);
-            if (meta.has_value()) {
+            if (ufvkId.has_value()) {
-                result = self->GetUnifiedAccountId(meta.value().GetUFVKId());
+                result = self->GetUnifiedAccountId(ufvkId.value());
             }
         },
         [&](const libzcash::UnifiedFullViewingKey& vk) {
@@ -2074,20 +2074,12 @@ bool CWallet::SelectorMatchesAddress(
             return false;
         },
         [&](const libzcash::UnifiedFullViewingKey& ufvk) {
-            std::optional<AddressUFVKMetadata> meta;
+            auto meta = self->GetUFVKMetadataForAddress(address);
-            std::visit(match {
-                [&](const CNoDestination& none) { meta = std::nullopt; },
-                [&](const auto& addr) { meta = self->GetUFVKMetadataForReceiver(addr); }
-            }, address);
             return (meta.has_value() && meta.value().GetUFVKId() == ufvk.GetKeyID(Params()));
         },
         [&](const AccountZTXOPattern& acct) {
             if (acct.IncludesP2PKH() || acct.IncludesP2SH()) {
-                std::optional<AddressUFVKMetadata> meta;
+                auto meta = self->GetUFVKMetadataForAddress(address);
-                std::visit(match {
-                    [&](const CNoDestination& none) { meta = std::nullopt; },
-                    [&](const auto& addr) { meta = self->GetUFVKMetadataForReceiver(addr); }
-                }, 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);
+    auto ufvkId = m_wallet->GetUFVKIdForAddress(uaddr);
-    if (ufvkMeta.has_value()) {
+    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<libzcash::ZcashdUnifiedFullViewingKey> 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<libzcash::UnifiedAddress> 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
+        diversifier_index_t j = ufvkMeta.value().GetDiversifierIndex();
-        // index at which the address was produced
-        assert(ufvkMeta.value().GetDiversifierIndex().has_value());
-        diversifier_index_t j = ufvkMeta.value().GetDiversifierIndex().value();
         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.");