From 18f8abb62de00318c69e2a270548e2bdbb554f79 Mon Sep 17 00:00:00 2001
From: Simon <simon@bitcartel.com>
Date: Fri, 21 Sep 2018 11:10:15 -0700
Subject: [PATCH 01/19] Closes #3534. Do not use APPROX_RELEASE_HEIGHT to get
 consensus branch id when in regtest mode.

Co-authored-by: Larry Ruane <larryruane@gmail.com>
---
 qa/pull-tester/rpc-tests.sh                |  1 +
 qa/rpc-tests/regtest_signrawtransaction.py | 34 ++++++++++++++++++++++
 src/rpc/rawtransaction.cpp                 |  7 +++--
 3 files changed, 40 insertions(+), 2 deletions(-)
 create mode 100755 qa/rpc-tests/regtest_signrawtransaction.py

diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh
index 301b822b8..1f25465fa 100755
--- a/qa/pull-tester/rpc-tests.sh
+++ b/qa/pull-tester/rpc-tests.sh
@@ -63,6 +63,7 @@ testScripts=(
     'rewind_index.py'
     'p2p_txexpiry_dos.py'
     'p2p_node_bloom.py'
+    'regtest_signrawtransaction.py'
 );
 testScriptsExt=(
     'getblocktemplate_longpoll.py'
diff --git a/qa/rpc-tests/regtest_signrawtransaction.py b/qa/rpc-tests/regtest_signrawtransaction.py
new file mode 100755
index 000000000..2e0273677
--- /dev/null
+++ b/qa/rpc-tests/regtest_signrawtransaction.py
@@ -0,0 +1,34 @@
+#!/usr/bin/env python2
+# Copyright (c) 2018 The Zcash developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import start_nodes, wait_and_assert_operationid_status
+
+class RegtestSignrawtransactionTest (BitcoinTestFramework):
+
+    def setup_nodes(self):
+        return start_nodes(4, self.options.tmpdir, [[
+            "-nuparams=5ba81b19:200", # Overwinter
+            "-nuparams=76b809bb:204", # Sapling
+        ]] * 4)
+
+    def run_test(self):
+        self.nodes[0].generate(1)
+        self.sync_all()
+        taddr = self.nodes[1].getnewaddress()
+        zaddr1 = self.nodes[1].z_getnewaddress('sprout')
+
+        self.nodes[0].sendtoaddress(taddr, 2.0)
+        self.nodes[0].generate(1)
+        self.sync_all()
+
+        # Create and sign Overwinter transaction.
+        # If the incorrect consensus branch id is selected, there will be a signing error. 
+        opid = self.nodes[1].z_sendmany(taddr,
+            [{'address': zaddr1, 'amount': 1}])
+        wait_and_assert_operationid_status(self.nodes[1], opid)
+
+if __name__ == '__main__':
+    RegtestSignrawtransactionTest().main()
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 242403b19..7d3dcc705 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -918,8 +918,11 @@ UniValue signrawtransaction(const UniValue& params, bool fHelp)
     bool fHashSingle = ((nHashType & ~SIGHASH_ANYONECANPAY) == SIGHASH_SINGLE);
     // Use the approximate release height if it is greater so offline nodes 
     // have a better estimation of the current height and will be more likely to
-    // determine the correct consensus branch ID.
-    int chainHeight = std::max(chainActive.Height() + 1, APPROX_RELEASE_HEIGHT);
+    // determine the correct consensus branch ID.  Regtest mode ignores release height.
+    int chainHeight = chainActive.Height() + 1;
+    if (Params().NetworkIDString() != "regtest") {
+        chainHeight = std::max(chainHeight, APPROX_RELEASE_HEIGHT);
+    }
     // Grab the current consensus branch ID
     auto consensusBranchId = CurrentEpochBranchId(chainHeight, Params().GetConsensus());
 

From f1cb49ac86dae3403bcd92689999ff256571573b Mon Sep 17 00:00:00 2001
From: Simon <simon@bitcartel.com>
Date: Fri, 21 Sep 2018 15:01:45 -0700
Subject: [PATCH 02/19] For #3533. Replace asserts with JSON errors.

This helps prevent users from triggering an assert if they pass in
Sapling addresses to RPC calls: z_mergetoaddress, z_importviewingkey
and z_exportviewingkey.
---
 src/wallet/asyncrpcoperation_mergetoaddress.cpp | 12 ++++++++----
 src/wallet/rpcdump.cpp                          | 12 ++++++++----
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/src/wallet/asyncrpcoperation_mergetoaddress.cpp b/src/wallet/asyncrpcoperation_mergetoaddress.cpp
index 6a16dd4fa..80968e01c 100644
--- a/src/wallet/asyncrpcoperation_mergetoaddress.cpp
+++ b/src/wallet/asyncrpcoperation_mergetoaddress.cpp
@@ -82,8 +82,10 @@ AsyncRPCOperation_mergetoaddress::AsyncRPCOperation_mergetoaddress(
         auto address = DecodePaymentAddress(std::get<0>(recipient));
         if (IsValidPaymentAddress(address)) {
             isToZaddr_ = true;
-            // TODO: Add Sapling support. For now, ensure we can later convert freely.
-            assert(boost::get<libzcash::SproutPaymentAddress>(&address) != nullptr);
+            // TODO: Add Sapling support. For now, return an error to the user.
+            if (boost::get<libzcash::SproutPaymentAddress>(&address) == nullptr) {
+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Currently, only Sprout zaddrs are supported");
+            }
             toPaymentAddress_ = address;
         } else {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid recipient address");
@@ -328,8 +330,10 @@ bool AsyncRPCOperation_mergetoaddress::main_impl()
     // Copy zinputs to more flexible containers
     std::deque<MergeToAddressInputNote> zInputsDeque;
     for (auto o : noteInputs_) {
-        // TODO: Add Sapling support. For now, ensure we can later convert freely.
-        assert(boost::get<libzcash::SproutSpendingKey>(&std::get<3>(o)) != nullptr);
+        // TODO: Add Sapling support. For now, return an error to the user.
+        if (boost::get<libzcash::SproutSpendingKey>(&std::get<3>(o)) == nullptr) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Currently, only Sprout zaddrs are supported");
+        }
         zInputsDeque.push_back(o);
     }
 
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index 81e2969d5..d507a3b1b 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -724,8 +724,10 @@ UniValue z_importviewingkey(const UniValue& params, bool fHelp)
     if (!IsValidViewingKey(viewingkey)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid viewing key");
     }
-    // TODO: Add Sapling support. For now, ensure we can freely convert.
-    assert(boost::get<libzcash::SproutViewingKey>(&viewingkey) != nullptr);
+    // TODO: Add Sapling support. For now, return an error to the user.
+    if (boost::get<libzcash::SproutViewingKey>(&viewingkey) == nullptr) {
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Currently, only Sprout viewing keys are supported");
+    }
     auto vkey = boost::get<libzcash::SproutViewingKey>(viewingkey);
     auto addr = vkey.address();
 
@@ -824,8 +826,10 @@ UniValue z_exportviewingkey(const UniValue& params, bool fHelp)
     if (!IsValidPaymentAddress(address)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid zaddr");
     }
-    // TODO: Add Sapling support. For now, ensure we can freely convert.
-    assert(boost::get<libzcash::SproutPaymentAddress>(&address) != nullptr);
+    // TODO: Add Sapling support. For now, return an error to the user.
+    if (boost::get<libzcash::SproutPaymentAddress>(&address) == nullptr) {
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Currently, only Sprout zaddrs are supported");
+    }
     auto addr = boost::get<libzcash::SproutPaymentAddress>(address);
 
     libzcash::SproutViewingKey vk;

From 34e222c18ebd51afdf175753b3759c9a4e571ae9 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@z.cash>
Date: Fri, 21 Sep 2018 23:43:31 +0100
Subject: [PATCH 03/19] Revert "Disable Sapling features on mainnet"

This reverts commit 554e00e8f9ca7edb8a34306e4f2b4a93e3319a01.
---
 src/gtest/test_keys.cpp       |  2 +-
 src/key_io.cpp                | 12 ++----------
 src/test/rpc_wallet_tests.cpp | 13 +++++--------
 src/wallet/rpcwallet.cpp      |  6 +-----
 4 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/src/gtest/test_keys.cpp b/src/gtest/test_keys.cpp
index 450bc5708..4b4ff8817 100644
--- a/src/gtest/test_keys.cpp
+++ b/src/gtest/test_keys.cpp
@@ -5,7 +5,7 @@
 
 #include <gtest/gtest.h>
 
-TEST(Keys, DISABLED_EncodeAndDecodeSapling)
+TEST(Keys, EncodeAndDecodeSapling)
 {
     SelectParams(CBaseChainParams::MAIN);
 
diff --git a/src/key_io.cpp b/src/key_io.cpp
index 636163638..7ffb61974 100644
--- a/src/key_io.cpp
+++ b/src/key_io.cpp
@@ -286,11 +286,7 @@ libzcash::PaymentAddress DecodePaymentAddress(const std::string& str)
     }
     data.clear();
     auto bech = bech32::Decode(str);
-    bool allowSapling = Params().NetworkIDString() == "regtest" || (
-        Params().NetworkIDString() == "test" &&
-        GetBoolArg("-experimentalfeatures", false) &&
-        GetBoolArg("-developersapling", false));
-    if (allowSapling && bech.first == Params().Bech32HRP(CChainParams::SAPLING_PAYMENT_ADDRESS) &&
+    if (bech.first == Params().Bech32HRP(CChainParams::SAPLING_PAYMENT_ADDRESS) &&
         bech.second.size() == ConvertedSaplingPaymentAddressSize) {
         // Bech32 decoding
         data.reserve((bech.second.size() * 5) / 8);
@@ -356,11 +352,7 @@ libzcash::SpendingKey DecodeSpendingKey(const std::string& str)
     }
     data.clear();
     auto bech = bech32::Decode(str);
-    bool allowSapling = Params().NetworkIDString() == "regtest" || (
-        Params().NetworkIDString() == "test" &&
-        GetBoolArg("-experimentalfeatures", false) &&
-        GetBoolArg("-developersapling", false));
-    if (allowSapling && bech.first == Params().Bech32HRP(CChainParams::SAPLING_EXTENDED_SPEND_KEY) &&
+    if (bech.first == Params().Bech32HRP(CChainParams::SAPLING_EXTENDED_SPEND_KEY) &&
         bech.second.size() == ConvertedSaplingExtendedSpendingKeySize) {
         // Bech32 decoding
         data.reserve((bech.second.size() * 5) / 8);
diff --git a/src/test/rpc_wallet_tests.cpp b/src/test/rpc_wallet_tests.cpp
index 6b132d0a1..811b98bab 100644
--- a/src/test/rpc_wallet_tests.cpp
+++ b/src/test/rpc_wallet_tests.cpp
@@ -383,13 +383,12 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_z_validateaddress)
     BOOST_CHECK_NO_THROW(retValue = CallRPC("z_validateaddress zs1z7rejlpsa98s2rrrfkwmaxu53e4ue0ulcrw0h4x5g8jl04tak0d3mm47vdtahatqrlkngh9slya"));
     resultObj = retValue.get_obj();
     b = find_value(resultObj, "isvalid").get_bool();
-    // TODO: Revert when we re-enable Sapling addresses on mainnet
+    BOOST_CHECK_EQUAL(b, true);
+    BOOST_CHECK_EQUAL(find_value(resultObj, "type").get_str(), "sapling");
+    b = find_value(resultObj, "ismine").get_bool();
     BOOST_CHECK_EQUAL(b, false);
-    // BOOST_CHECK_EQUAL(find_value(resultObj, "type").get_str(), "sapling");
-    // b = find_value(resultObj, "ismine").get_bool();
-    // BOOST_CHECK_EQUAL(b, false);
-    // BOOST_CHECK_EQUAL(find_value(resultObj, "diversifier").get_str(), "1787997c30e94f050c634d");
-    // BOOST_CHECK_EQUAL(find_value(resultObj, "diversifiedtransmissionkey").get_str(), "34ed1f60f5db5763beee1ddbb37dd5f7e541d4d4fbdcc09fbfcc6b8e949bbe9d");
+    BOOST_CHECK_EQUAL(find_value(resultObj, "diversifier").get_str(), "1787997c30e94f050c634d");
+    BOOST_CHECK_EQUAL(find_value(resultObj, "diversifiedtransmissionkey").get_str(), "34ed1f60f5db5763beee1ddbb37dd5f7e541d4d4fbdcc09fbfcc6b8e949bbe9d");
 }
 
 /*
@@ -537,8 +536,6 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_z_importwallet)
  */
 BOOST_AUTO_TEST_CASE(rpc_wallet_z_importexport)
 {
-    SelectParams(CBaseChainParams::REGTEST);
-
     LOCK2(cs_main, pwalletMain->cs_wallet);
     UniValue retValue;
     int n1 = 1000; // number of times to import/export
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 39d019b92..82b98304a 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -3141,13 +3141,9 @@ UniValue z_getnewaddress(const UniValue& params, bool fHelp)
         addrType = params[0].get_str();
     }
 
-    bool allowSapling = Params().NetworkIDString() == "regtest" || (
-        Params().NetworkIDString() == "test" &&
-        GetBoolArg("-experimentalfeatures", false) &&
-        GetBoolArg("-developersapling", false));
     if (addrType == ADDR_TYPE_SPROUT) {
         return EncodePaymentAddress(pwalletMain->GenerateNewZKey());
-    } else if (addrType == ADDR_TYPE_SAPLING && allowSapling) {
+    } else if (addrType == ADDR_TYPE_SAPLING) {
         return EncodePaymentAddress(pwalletMain->GenerateNewSaplingZKey());
     } else {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid address type");

From 4c4e1718b123c296a5ebeaddf866dab9082b2531 Mon Sep 17 00:00:00 2001
From: Simon <simon@bitcartel.com>
Date: Fri, 21 Sep 2018 17:16:44 -0700
Subject: [PATCH 04/19] Update qa test as offline regtest nodes need branch id
 passed in.

---
 qa/rpc-tests/signrawtransaction_offline.py | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/qa/rpc-tests/signrawtransaction_offline.py b/qa/rpc-tests/signrawtransaction_offline.py
index cde9ca166..0d7def43b 100755
--- a/qa/rpc-tests/signrawtransaction_offline.py
+++ b/qa/rpc-tests/signrawtransaction_offline.py
@@ -2,6 +2,7 @@
 
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal, assert_true, initialize_chain_clean, start_node
+from test_framework.authproxy import JSONRPCException
 
 class SignOfflineTest (BitcoinTestFramework):
     # Setup Methods
@@ -36,7 +37,17 @@ class SignOfflineTest (BitcoinTestFramework):
 
         create_hex = self.nodes[0].createrawtransaction(create_inputs, {taddr: 9.9999})
 
-        signed_tx = offline_node.signrawtransaction(create_hex, sign_inputs, privkeys)
+        # An offline regtest node does not rely on the approx release height of the software
+        # to determine the consensus rules to be used for signing.
+        try:
+            signed_tx = offline_node.signrawtransaction(create_hex, sign_inputs, privkeys)
+            self.nodes[0].sendrawtransaction(signed_tx['hex'])
+            assert(False)
+        except JSONRPCException:
+            pass
+
+        # Passing in the consensus branch id resolves the issue for offline regtest nodes.
+        signed_tx = offline_node.signrawtransaction(create_hex, sign_inputs, privkeys, "ALL", "5ba81b19")
 
         # If we return the transaction hash, then we have have not thrown an error (success)
         online_tx_hash = self.nodes[0].sendrawtransaction(signed_tx['hex'])

From 83c4e360daba7ea9e3a6c24a1d65acf656819b2f Mon Sep 17 00:00:00 2001
From: Eirik Ogilvie-Wigley <eirik@z.cash>
Date: Thu, 27 Sep 2018 15:44:04 -0600
Subject: [PATCH 05/19] Address need not be optional when adding sapling keys

---
 src/gtest/test_keystore.cpp      |  6 ------
 src/keystore.cpp                 | 14 ++++++--------
 src/keystore.h                   |  8 ++++----
 src/wallet/crypter.cpp           |  8 ++++----
 src/wallet/crypter.h             |  4 ++--
 src/wallet/gtest/test_wallet.cpp | 16 ++++++++--------
 src/wallet/wallet.cpp            |  4 ++--
 src/wallet/wallet.h              |  4 ++--
 8 files changed, 28 insertions(+), 36 deletions(-)

diff --git a/src/gtest/test_keystore.cpp b/src/gtest/test_keystore.cpp
index 6b4d08a59..ccf9cb9ba 100644
--- a/src/gtest/test_keystore.cpp
+++ b/src/gtest/test_keystore.cpp
@@ -215,12 +215,6 @@ TEST(keystore_tests, StoreAndRetrieveSaplingSpendingKey) {
     EXPECT_FALSE(keyStore.HaveSaplingIncomingViewingKey(addr));
     EXPECT_FALSE(keyStore.GetSaplingIncomingViewingKey(addr, ivkOut));
 
-    // If we don't specify the default address, that mapping isn't created
-    keyStore.AddSaplingSpendingKey(sk);
-    EXPECT_TRUE(keyStore.HaveSaplingSpendingKey(fvk));
-    EXPECT_TRUE(keyStore.HaveSaplingFullViewingKey(ivk));
-    EXPECT_FALSE(keyStore.HaveSaplingIncomingViewingKey(addr));
-
     // When we specify the default address, we get the full mapping
     keyStore.AddSaplingSpendingKey(sk, addr);
     EXPECT_TRUE(keyStore.HaveSaplingSpendingKey(fvk));
diff --git a/src/keystore.cpp b/src/keystore.cpp
index 8262c8aab..be5e0b82f 100644
--- a/src/keystore.cpp
+++ b/src/keystore.cpp
@@ -125,13 +125,13 @@ bool CBasicKeyStore::AddSproutSpendingKey(const libzcash::SproutSpendingKey &sk)
 //! Sapling 
 bool CBasicKeyStore::AddSaplingSpendingKey(
     const libzcash::SaplingExtendedSpendingKey &sk,
-    const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr)
+    const libzcash::SaplingPaymentAddress &defaultAddr)
 {
     LOCK(cs_SpendingKeyStore);
     auto fvk = sk.expsk.full_viewing_key();
 
     // if SaplingFullViewingKey is not in SaplingFullViewingKeyMap, add it
-    if (!AddSaplingFullViewingKey(fvk, defaultAddr)){
+    if (!AddSaplingFullViewingKey(fvk, defaultAddr)) {
         return false;
     }
 
@@ -151,17 +151,15 @@ bool CBasicKeyStore::AddSproutViewingKey(const libzcash::SproutViewingKey &vk)
 
 bool CBasicKeyStore::AddSaplingFullViewingKey(
     const libzcash::SaplingFullViewingKey &fvk,
-    const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr)
+    const libzcash::SaplingPaymentAddress &defaultAddr)
 {
     LOCK(cs_SpendingKeyStore);
     auto ivk = fvk.in_viewing_key();
     mapSaplingFullViewingKeys[ivk] = fvk;
-
-    if (defaultAddr) {
-        // Add defaultAddr -> SaplingIncomingViewing to SaplingIncomingViewingKeyMap
-        mapSaplingIncomingViewingKeys[defaultAddr.get()] = ivk;
-    }
     
+    // Add defaultAddr -> SaplingIncomingViewing to SaplingIncomingViewingKeyMap
+    mapSaplingIncomingViewingKeys[defaultAddr] = ivk;
+
     return true;
 }
 
diff --git a/src/keystore.h b/src/keystore.h
index a8a8f62f5..5b064f27e 100644
--- a/src/keystore.h
+++ b/src/keystore.h
@@ -66,7 +66,7 @@ public:
     //! Add a Sapling spending key to the store.
     virtual bool AddSaplingSpendingKey(
         const libzcash::SaplingExtendedSpendingKey &sk,
-        const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr = boost::none) =0;
+        const libzcash::SaplingPaymentAddress &defaultAddr) =0;
     
     //! Check whether a Sapling spending key corresponding to a given Sapling viewing key is present in the store.
     virtual bool HaveSaplingSpendingKey(const libzcash::SaplingFullViewingKey &fvk) const =0;
@@ -75,7 +75,7 @@ public:
     //! Support for Sapling full viewing keys
     virtual bool AddSaplingFullViewingKey(
         const libzcash::SaplingFullViewingKey &fvk,
-        const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr = boost::none) =0;
+        const libzcash::SaplingPaymentAddress &defaultAddr) =0;
     virtual bool HaveSaplingFullViewingKey(const libzcash::SaplingIncomingViewingKey &ivk) const =0;
     virtual bool GetSaplingFullViewingKey(
         const libzcash::SaplingIncomingViewingKey &ivk, 
@@ -236,7 +236,7 @@ public:
     //! Sapling 
     bool AddSaplingSpendingKey(
         const libzcash::SaplingExtendedSpendingKey &sk,
-        const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr = boost::none);
+        const libzcash::SaplingPaymentAddress &defaultAddr);
     bool HaveSaplingSpendingKey(const libzcash::SaplingFullViewingKey &fvk) const
     {
         bool result;
@@ -263,7 +263,7 @@ public:
 
     virtual bool AddSaplingFullViewingKey(
         const libzcash::SaplingFullViewingKey &fvk,
-        const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr = boost::none);
+        const libzcash::SaplingPaymentAddress &defaultAddr);
     virtual bool HaveSaplingFullViewingKey(const libzcash::SaplingIncomingViewingKey &ivk) const;
     virtual bool GetSaplingFullViewingKey(
         const libzcash::SaplingIncomingViewingKey &ivk, 
diff --git a/src/wallet/crypter.cpp b/src/wallet/crypter.cpp
index 88807598a..25d69cc3d 100644
--- a/src/wallet/crypter.cpp
+++ b/src/wallet/crypter.cpp
@@ -449,7 +449,7 @@ bool CCryptoKeyStore::AddSproutSpendingKey(const libzcash::SproutSpendingKey &sk
 
 bool CCryptoKeyStore::AddSaplingSpendingKey(
     const libzcash::SaplingExtendedSpendingKey &sk,
-    const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr)
+    const libzcash::SaplingPaymentAddress &defaultAddr)
 {
     {
         LOCK(cs_SpendingKeyStore);
@@ -496,7 +496,7 @@ bool CCryptoKeyStore::AddCryptedSproutSpendingKey(
 bool CCryptoKeyStore::AddCryptedSaplingSpendingKey(
     const libzcash::SaplingFullViewingKey &fvk,
     const std::vector<unsigned char> &vchCryptedSecret,
-    const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr)
+    const libzcash::SaplingPaymentAddress &defaultAddr)
 {
     {
         LOCK(cs_SpendingKeyStore);
@@ -505,7 +505,7 @@ bool CCryptoKeyStore::AddCryptedSaplingSpendingKey(
         }
 
         // if SaplingFullViewingKey is not in SaplingFullViewingKeyMap, add it
-        if (!AddSaplingFullViewingKey(fvk, defaultAddr)){
+        if (!AddSaplingFullViewingKey(fvk, defaultAddr)) {
             return false;
         }
 
@@ -614,7 +614,7 @@ bool CCryptoKeyStore::EncryptKeys(CKeyingMaterial& vMasterKeyIn)
             if (!EncryptSecret(vMasterKeyIn, vchSecret, fvk.GetFingerprint(), vchCryptedSecret)) {
                 return false;
             }
-            if (!AddCryptedSaplingSpendingKey(fvk, vchCryptedSecret)) {
+            if (!AddCryptedSaplingSpendingKey(fvk, vchCryptedSecret, sk.DefaultAddress())) {
                 return false;
             }
         }
diff --git a/src/wallet/crypter.h b/src/wallet/crypter.h
index 7c326fbc4..b751ce300 100644
--- a/src/wallet/crypter.h
+++ b/src/wallet/crypter.h
@@ -243,10 +243,10 @@ public:
     virtual bool AddCryptedSaplingSpendingKey(
         const libzcash::SaplingFullViewingKey &fvk,
         const std::vector<unsigned char> &vchCryptedSecret,
-        const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr = boost::none);
+        const libzcash::SaplingPaymentAddress &defaultAddr);
     bool AddSaplingSpendingKey(
         const libzcash::SaplingExtendedSpendingKey &sk,
-        const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr = boost::none);
+        const libzcash::SaplingPaymentAddress &defaultAddr);
     bool HaveSaplingSpendingKey(const libzcash::SaplingFullViewingKey &fvk) const
     {
         {
diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp
index a67c3b491..fa066061d 100644
--- a/src/wallet/gtest/test_wallet.cpp
+++ b/src/wallet/gtest/test_wallet.cpp
@@ -516,7 +516,7 @@ TEST(WalletTests, FindMySaplingNotes) {
     EXPECT_EQ(0, noteMap.size());
 
     // Add spending key to wallet, so Sapling notes can be found
-    ASSERT_TRUE(wallet.AddSaplingZKey(sk));
+    ASSERT_TRUE(wallet.AddSaplingZKey(sk, pk));
     ASSERT_TRUE(wallet.HaveSaplingSpendingKey(fvk));
     noteMap = wallet.FindMySaplingNotes(wtx);
     EXPECT_EQ(2, noteMap.size());
@@ -630,7 +630,7 @@ TEST(WalletTests, GetConflictedSaplingNotes) {
     auto ivk = fvk.in_viewing_key();
     auto pk = sk.DefaultAddress();
 
-    ASSERT_TRUE(wallet.AddSaplingZKey(sk));
+    ASSERT_TRUE(wallet.AddSaplingZKey(sk, pk));
     ASSERT_TRUE(wallet.HaveSaplingSpendingKey(fvk));
 
     // Generate note A
@@ -815,7 +815,7 @@ TEST(WalletTests, SaplingNullifierIsSpent) {
     auto tx = maybe_tx.get();
 
     CWalletTx wtx {&wallet, tx};
-    ASSERT_TRUE(wallet.AddSaplingZKey(sk));
+    ASSERT_TRUE(wallet.AddSaplingZKey(sk, pk));
     ASSERT_TRUE(wallet.HaveSaplingSpendingKey(fvk));
 
     // Manually compute the nullifier based on the known position
@@ -912,7 +912,7 @@ TEST(WalletTests, NavigateFromSaplingNullifierToNote) {
     auto tx = maybe_tx.get();
 
     CWalletTx wtx {&wallet, tx};
-    ASSERT_TRUE(wallet.AddSaplingZKey(sk));
+    ASSERT_TRUE(wallet.AddSaplingZKey(sk, pk));
     ASSERT_TRUE(wallet.HaveSaplingSpendingKey(fvk));
 
     // Manually compute the nullifier based on the expected position
@@ -1048,7 +1048,7 @@ TEST(WalletTests, SpentSaplingNoteIsFromMe) {
     auto tx = maybe_tx.get();
 
     CWalletTx wtx {&wallet, tx};
-    ASSERT_TRUE(wallet.AddSaplingZKey(sk));
+    ASSERT_TRUE(wallet.AddSaplingZKey(sk, pk));
     ASSERT_TRUE(wallet.HaveSaplingSpendingKey(fvk));
 
     // Fake-mine the transaction
@@ -1751,7 +1751,7 @@ TEST(WalletTests, UpdatedSaplingNoteData) {
 
     // Wallet contains fvk1 but not fvk2
     CWalletTx wtx {&wallet, tx};
-    ASSERT_TRUE(wallet.AddSaplingZKey(sk));
+    ASSERT_TRUE(wallet.AddSaplingZKey(sk, pk));
     ASSERT_TRUE(wallet.HaveSaplingSpendingKey(fvk));
     ASSERT_FALSE(wallet.HaveSaplingSpendingKey(fvk2));
 
@@ -1784,7 +1784,7 @@ TEST(WalletTests, UpdatedSaplingNoteData) {
     wtx = wallet.mapWallet[hash];
 
     // Now lets add key fvk2 so wallet can find the payment note sent to pk2
-    ASSERT_TRUE(wallet.AddSaplingZKey(sk2));
+    ASSERT_TRUE(wallet.AddSaplingZKey(sk2, pk2));
     ASSERT_TRUE(wallet.HaveSaplingSpendingKey(fvk2));
     CWalletTx wtx2 = wtx;
     auto saplingNoteData2 = wallet.FindMySaplingNotes(wtx2);
@@ -1881,7 +1881,7 @@ TEST(WalletTests, MarkAffectedSaplingTransactionsDirty) {
     auto ivk = fvk.in_viewing_key();
     auto pk = sk.DefaultAddress();
 
-    ASSERT_TRUE(wallet.AddSaplingZKey(sk));
+    ASSERT_TRUE(wallet.AddSaplingZKey(sk, pk));
     ASSERT_TRUE(wallet.HaveSaplingSpendingKey(fvk));
 
     // Set up transparent address
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index d931f7384..2093fadb9 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -150,7 +150,7 @@ SaplingPaymentAddress CWallet::GenerateNewSaplingZKey()
 // Add spending key to keystore 
 bool CWallet::AddSaplingZKey(
     const libzcash::SaplingExtendedSpendingKey &sk,
-    const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr)
+    const libzcash::SaplingPaymentAddress &defaultAddr)
 {
     AssertLockHeld(cs_wallet); // mapSaplingZKeyMetadata
 
@@ -291,7 +291,7 @@ bool CWallet::AddCryptedSproutSpendingKey(
 
 bool CWallet::AddCryptedSaplingSpendingKey(const libzcash::SaplingFullViewingKey &fvk,
                                            const std::vector<unsigned char> &vchCryptedSecret,
-                                           const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr)
+                                           const libzcash::SaplingPaymentAddress &defaultAddr)
 {
     if (!CCryptoKeyStore::AddCryptedSaplingSpendingKey(fvk, vchCryptedSecret, defaultAddr))
         return false;
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index dda1e4dfe..1deb9b3aa 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1058,11 +1058,11 @@ public:
     //! Adds Sapling spending key to the store, and saves it to disk
     bool AddSaplingZKey(
         const libzcash::SaplingExtendedSpendingKey &key,
-        const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr = boost::none);
+        const libzcash::SaplingPaymentAddress &defaultAddr);
     bool AddCryptedSaplingSpendingKey(
         const libzcash::SaplingFullViewingKey &fvk,
         const std::vector<unsigned char> &vchCryptedSecret,
-        const boost::optional<libzcash::SaplingPaymentAddress> &defaultAddr = boost::none);
+        const libzcash::SaplingPaymentAddress &defaultAddr);
 
     /** 
      * Increment the next transaction order id

From e4f0d6a8de9316355430043785807bfc9765c5da Mon Sep 17 00:00:00 2001
From: Larry Ruane <larry@z.cash>
Date: Tue, 25 Sep 2018 13:48:01 -0600
Subject: [PATCH 06/19] Sapling support for z_listreceivedbyaddress

---
 qa/pull-tester/rpc-tests.sh         |   1 +
 qa/rpc-tests/wallet_listreceived.py | 101 ++++++++++++++++++++++++++++
 src/wallet/rpcwallet.cpp            |  57 ++++++++++------
 src/wallet/wallet.cpp               |  58 ++++++++++++++--
 src/wallet/wallet.h                 |   3 +-
 5 files changed, 194 insertions(+), 26 deletions(-)
 create mode 100755 qa/rpc-tests/wallet_listreceived.py

diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh
index 1f25465fa..dde4144c4 100755
--- a/qa/pull-tester/rpc-tests.sh
+++ b/qa/pull-tester/rpc-tests.sh
@@ -19,6 +19,7 @@ testScripts=(
     'wallet_import_export.py'
     'wallet_protectcoinbase.py'
     'wallet_shieldcoinbase.py'
+    'wallet_listreceived.py'
     'wallet_mergetoaddress.py'
     'wallet.py'
     'wallet_overwintertx.py'
diff --git a/qa/rpc-tests/wallet_listreceived.py b/qa/rpc-tests/wallet_listreceived.py
new file mode 100755
index 000000000..88d4f8cc7
--- /dev/null
+++ b/qa/rpc-tests/wallet_listreceived.py
@@ -0,0 +1,101 @@
+#!/usr/bin/env python2
+# Copyright (c) 2018 The Zcash developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal, assert_true, assert_false
+from test_framework.util import start_nodes, wait_and_assert_operationid_status
+from decimal import Decimal
+
+my_memo = 'c0ffee' # stay awake
+my_memo = my_memo + '0'*(1024-len(my_memo))
+
+no_memo = 'f6' + ('0'*1022) # see section 5.5 of the protocol spec
+# sapling generates zero_memo, but this may be fixed soon (to no_memo)
+# then this test can be simplified
+zero_memo = '0'*1024
+
+fee = Decimal('0.0001')
+
+class ListReceivedTest (BitcoinTestFramework):
+
+    def setup_nodes(self):
+        return start_nodes(4, self.options.tmpdir, [[
+            "-nuparams=5ba81b19:201", # Overwinter
+            "-nuparams=76b809bb:204", # Sapling
+        ]] * 4)
+
+    def generate_and_sync(self, new_height):
+        self.nodes[0].generate(1)
+        self.sync_all()
+        assert_equal(new_height, self.nodes[0].getblockcount())
+
+    def run_test_release(self, release, expected_memo, height):
+        self.generate_and_sync(height+1)
+        taddr = self.nodes[1].getnewaddress()
+        zaddr1 = self.nodes[1].z_getnewaddress(release)
+
+        self.nodes[0].sendtoaddress(taddr, 2.0)
+        self.generate_and_sync(height+2)
+
+        # Send 1 ZEC to zaddr1
+        opid = self.nodes[1].z_sendmany(taddr,
+            [{'address': zaddr1, 'amount': 1, 'memo': my_memo}])
+        txid = wait_and_assert_operationid_status(self.nodes[1], opid)
+        self.sync_all()
+        r = self.nodes[1].z_listreceivedbyaddress(zaddr1)
+        assert_equal(0, len(r), "Should have received no confirmed note")
+
+        # No confirmation required, one note should be present
+        r = self.nodes[1].z_listreceivedbyaddress(zaddr1, 0)
+        assert_equal(1, len(r), "Should have received one (unconfirmed) note")
+        assert_equal(txid, r[0]['txid'])
+        assert_equal(1, r[0]['amount'])
+        assert_false(r[0]['change'], "Note should not be change")
+        assert_equal(my_memo, r[0]['memo'])
+
+        # Confirm transaction (1 ZEC from taddr to zaddr1)
+        self.generate_and_sync(height+3)
+
+        # Require one confirmation, note should be present
+        assert_equal(r, self.nodes[1].z_listreceivedbyaddress(zaddr1))
+
+        # Generate some change by sending part of zaddr1 to zaddr2
+        zaddr2 = self.nodes[1].z_getnewaddress(release)
+        opid = self.nodes[1].z_sendmany(zaddr1,
+            [{'address': zaddr2, 'amount': 0.6, 'memo': my_memo}])
+        txid = wait_and_assert_operationid_status(self.nodes[1], opid)
+        self.sync_all()
+        self.generate_and_sync(height+4)
+
+        # zaddr1 should have a note with change
+        r = self.nodes[1].z_listreceivedbyaddress(zaddr1, 0)
+        r = sorted(r, key = lambda received: received['amount'])
+        assert_equal(2, len(r), "zaddr1 Should have received 2 notes")
+
+        assert_equal(txid, r[0]['txid'])
+        assert_equal(Decimal('0.4')-fee, r[0]['amount'])
+        assert_true(r[0]['change'], "Note valued at (0.4-fee) should be change")
+        assert_equal(expected_memo, r[0]['memo'])
+
+        # The old note still exists (it's immutable), even though it is spent
+        assert_equal(Decimal('1.0'), r[1]['amount'])
+        assert_false(r[1]['change'], "Note valued at 1.0 should not be change")
+        assert_equal(expected_memo, r[0]['memo'])
+
+        # zaddr2 should not have change
+        r = self.nodes[1].z_listreceivedbyaddress(zaddr2, 0)
+        r = sorted(r, key = lambda received: received['amount'])
+        assert_equal(1, len(r), "zaddr2 Should have received 1 notes")
+        assert_equal(txid, r[0]['txid'])
+        assert_equal(Decimal('0.6'), r[0]['amount'])
+        assert_false(r[0]['change'], "Note valued at 0.6 should not be change")
+        assert_equal(my_memo, r[0]['memo'])
+
+    def run_test(self):
+        self.run_test_release('sprout', no_memo, 200)
+        self.run_test_release('sapling', zero_memo, 204)
+
+if __name__ == '__main__':
+    ListReceivedTest().main()
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 82b98304a..d93279986 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -2579,7 +2579,7 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
             std::string data(entry.plaintext.memo().begin(), entry.plaintext.memo().end());
             obj.push_back(Pair("memo", HexStr(data)));
             if (hasSproutSpendingKey) {
-                obj.push_back(Pair("change", pwalletMain->IsNoteChange(nullifierSet, entry.address, entry.jsop)));
+                obj.push_back(Pair("change", pwalletMain->IsNoteSproutChange(nullifierSet, entry.address, entry.jsop)));
             }
             results.push_back(obj);
         }
@@ -3307,12 +3307,9 @@ UniValue z_listreceivedbyaddress(const UniValue& params, bool fHelp)
     if (!IsValidPaymentAddress(zaddr)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid zaddr.");
     }
-    // TODO: Add Sapling support. For now, ensure we can freely convert.
-    assert(boost::get<libzcash::SproutPaymentAddress>(&zaddr) != nullptr);
-    auto sproutzaddr = boost::get<libzcash::SproutPaymentAddress>(zaddr);
 
-    bool hasSproutSpendingKey = pwalletMain->HaveSproutSpendingKey(sproutzaddr);
-    if (!(hasSproutSpendingKey || pwalletMain->HaveSproutViewingKey(sproutzaddr))) {
+    // Visitor to support Sprout and Sapling addrs
+    if (!boost::apply_visitor(PaymentAddressBelongsToWallet(pwalletMain), zaddr)) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "From address does not belong to this node, zaddr spending key or viewing key not found.");
     }
 
@@ -3320,22 +3317,40 @@ UniValue z_listreceivedbyaddress(const UniValue& params, bool fHelp)
     std::vector<CSproutNotePlaintextEntry> sproutEntries;
     std::vector<SaplingNoteEntry> saplingEntries;
     pwalletMain->GetFilteredNotes(sproutEntries, saplingEntries, fromaddress, nMinDepth, false, false);
-    auto nullifierSet = hasSproutSpendingKey ? pwalletMain->GetNullifiersForAddresses({zaddr}) : std::set<std::pair<PaymentAddress, uint256>>();
-    for (CSproutNotePlaintextEntry & entry : sproutEntries) {
-        UniValue obj(UniValue::VOBJ);
-        obj.push_back(Pair("txid", entry.jsop.hash.ToString()));
-        obj.push_back(Pair("amount", ValueFromAmount(CAmount(entry.plaintext.value()))));
-        std::string data(entry.plaintext.memo().begin(), entry.plaintext.memo().end());
-        obj.push_back(Pair("memo", HexStr(data)));
-        // (txid, jsindex, jsoutindex) is needed to globally identify a note
-        obj.push_back(Pair("jsindex", entry.jsop.js));
-        obj.push_back(Pair("jsoutindex", entry.jsop.n));
-        if (hasSproutSpendingKey) {
-            obj.push_back(Pair("change", pwalletMain->IsNoteChange(nullifierSet, entry.address, entry.jsop)));
-        }
-        result.push_back(obj);
+
+    std::set<std::pair<PaymentAddress, uint256>> nullifierSet;
+    auto hasSpendingKey = boost::apply_visitor(HaveSpendingKeyForPaymentAddress(pwalletMain), zaddr);
+    if (hasSpendingKey) {
+        nullifierSet = pwalletMain->GetNullifiersForAddresses({zaddr});
+    }
+
+    if (boost::get<libzcash::SproutPaymentAddress>(&zaddr) != nullptr) {
+        for (CSproutNotePlaintextEntry & entry : sproutEntries) {
+            UniValue obj(UniValue::VOBJ);
+            obj.push_back(Pair("txid", entry.jsop.hash.ToString()));
+            obj.push_back(Pair("amount", ValueFromAmount(CAmount(entry.plaintext.value()))));
+            std::string data(entry.plaintext.memo().begin(), entry.plaintext.memo().end());
+            obj.push_back(Pair("memo", HexStr(data)));
+            obj.push_back(Pair("jsindex", entry.jsop.js));
+            obj.push_back(Pair("jsoutindex", entry.jsop.n));
+            if (hasSpendingKey) {
+                obj.push_back(Pair("change", pwalletMain->IsNoteSproutChange(nullifierSet, entry.address, entry.jsop)));
+            }
+            result.push_back(obj);
+        }
+    } else if (boost::get<libzcash::SaplingPaymentAddress>(&zaddr) != nullptr) {
+        for (SaplingNoteEntry & entry : saplingEntries) {
+            UniValue obj(UniValue::VOBJ);
+            obj.push_back(Pair("txid", entry.op.hash.ToString()));
+            obj.push_back(Pair("amount", ValueFromAmount(CAmount(entry.note.value()))));
+            obj.push_back(Pair("memo", HexStr(entry.memo)));
+            obj.push_back(Pair("outindex", (int)entry.op.n));
+            if (hasSpendingKey) {
+              obj.push_back(Pair("change", pwalletMain->IsNoteSaplingChange(nullifierSet, entry.address, entry.op)));
+            }
+            result.push_back(obj);
+        }
     }
-    // TODO: Sapling
     return result;
 }
 
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 6e2631db3..d838f485e 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -512,20 +512,50 @@ void CWallet::SetBestChain(const CBlockLocator& loc)
     SetBestChainINTERNAL(walletdb, loc);
 }
 
-std::set<std::pair<libzcash::PaymentAddress, uint256>> CWallet::GetNullifiersForAddresses(const std::set<libzcash::PaymentAddress> & addresses)
+std::set<std::pair<libzcash::PaymentAddress, uint256>> CWallet::GetNullifiersForAddresses(
+        const std::set<libzcash::PaymentAddress> & addresses)
 {
     std::set<std::pair<libzcash::PaymentAddress, uint256>> nullifierSet;
+    // Sapling ivk -> list of addrs map
+    // (There may be more than one diversified address for a given ivk.)
+    std::map<libzcash::SaplingIncomingViewingKey, std::vector<libzcash::SaplingPaymentAddress>> ivkMap;
+    for (const auto & addr : addresses) {
+        auto saplingAddr = boost::get<libzcash::SaplingPaymentAddress>(&addr);
+        if (saplingAddr != nullptr) {
+            libzcash::SaplingIncomingViewingKey ivk;
+            this->GetSaplingIncomingViewingKey(*saplingAddr, ivk);
+            ivkMap[ivk].push_back(*saplingAddr);
+        }
+    }
     for (const auto & txPair : mapWallet) {
+        // Sprout
         for (const auto & noteDataPair : txPair.second.mapSproutNoteData) {
-            if (noteDataPair.second.nullifier && addresses.count(noteDataPair.second.address)) {
-                nullifierSet.insert(std::make_pair(noteDataPair.second.address, noteDataPair.second.nullifier.get()));
+            auto & noteData = noteDataPair.second;
+            auto & nullifier = noteData.nullifier;
+            auto & address = noteData.address;
+            if (nullifier && addresses.count(address)) {
+                nullifierSet.insert(std::make_pair(address, nullifier.get()));
+            }
+        }
+        // Sapling
+        for (const auto & noteDataPair : txPair.second.mapSaplingNoteData) {
+            auto & noteData = noteDataPair.second;
+            auto & nullifier = noteData.nullifier;
+            auto & ivk = noteData.ivk;
+            if (nullifier && ivkMap.count(ivk)) {
+                for (const auto & addr : ivkMap[ivk]) {
+                    nullifierSet.insert(std::make_pair(addr, nullifier.get()));
+                }
             }
         }
     }
     return nullifierSet;
 }
 
-bool CWallet::IsNoteChange(const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet, const PaymentAddress & address, const JSOutPoint & jsop)
+bool CWallet::IsNoteSproutChange(
+        const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet,
+        const PaymentAddress & address,
+        const JSOutPoint & jsop)
 {
     // A Note is marked as "change" if the address that received it
     // also spent Notes in the same transaction. This will catch,
@@ -546,6 +576,26 @@ bool CWallet::IsNoteChange(const std::set<std::pair<libzcash::PaymentAddress, ui
     return false;
 }
 
+bool CWallet::IsNoteSaplingChange(const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet,
+        const libzcash::PaymentAddress & address,
+        const SaplingOutPoint & op)
+{
+    // A Note is marked as "change" if the address that received it
+    // also spent Notes in the same transaction. This will catch,
+    // for instance:
+    // - Change created by spending fractions of Notes (because
+    //   z_sendmany sends change to the originating z-address).
+    // - Notes created by consolidation transactions (e.g. using
+    //   z_mergetoaddress).
+    // - Notes sent from one address to itself.
+    for (const SpendDescription &spend : mapWallet[op.hash].vShieldedSpend) {
+        if (nullifierSet.count(std::make_pair(address, spend.nullifier))) {
+            return true;
+        }
+    }
+    return false;
+}
+
 bool CWallet::SetMinVersion(enum WalletFeature nVersion, CWalletDB* pwalletdbIn, bool fExplicit)
 {
     LOCK(cs_wallet); // nWalletVersion
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 50acb2394..3589f9629 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1161,7 +1161,8 @@ public:
     /** Saves witness caches and best block locator to disk. */
     void SetBestChain(const CBlockLocator& loc);
     std::set<std::pair<libzcash::PaymentAddress, uint256>> GetNullifiersForAddresses(const std::set<libzcash::PaymentAddress> & addresses);
-    bool IsNoteChange(const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet, const libzcash::PaymentAddress & address, const JSOutPoint & entry);
+    bool IsNoteSproutChange(const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet, const libzcash::PaymentAddress & address, const JSOutPoint & entry);
+    bool IsNoteSaplingChange(const std::set<std::pair<libzcash::PaymentAddress, uint256>> & nullifierSet, const libzcash::PaymentAddress & address, const SaplingOutPoint & entry);
 
     DBErrors LoadWallet(bool& fFirstRunRet);
     DBErrors ZapWalletTx(std::vector<CWalletTx>& vWtx);

From a13492744dee59a831633acf0faeb7cf0575d05d Mon Sep 17 00:00:00 2001
From: Eirik Ogilvie-Wigley <eirik@z.cash>
Date: Fri, 28 Sep 2018 10:32:30 -0600
Subject: [PATCH 07/19] Use max priority for all shielded transfers

---
 src/coins.cpp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/coins.cpp b/src/coins.cpp
index 257de700b..edc4d2e20 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -616,13 +616,13 @@ double CCoinsViewCache::GetPriority(const CTransaction &tx, int nHeight) const
     if (tx.IsCoinBase())
         return 0.0;
 
-    // Joinsplits do not reveal any information about the value or age of a note, so we
+    // Shielded transfers do not reveal any information about the value or age of a note, so we
     // cannot apply the priority algorithm used for transparent utxos.  Instead, we just
-    // use the maximum priority whenever a transaction contains any JoinSplits.
-    // (Note that coinbase transactions cannot contain JoinSplits.)
+    // use the maximum priority for all (partially or fully) shielded transactions.
+    // (Note that coinbase transactions cannot contain JoinSplits, or Sapling shielded Spends or Outputs.)
     // FIXME: this logic is partially duplicated between here and CreateNewBlock in miner.cpp.
 
-    if (tx.vjoinsplit.size() > 0) {
+    if (tx.vjoinsplit.size() > 0 || tx.vShieldedSpend.size() > 0 || tx.vShieldedOutput.size() > 0) {
         return MAX_PRIORITY;
     }
 

From 51e6ed6110e612d02698a855b0b1b2027e2c5538 Mon Sep 17 00:00:00 2001
From: Eirik Ogilvie-Wigley <eirik@z.cash>
Date: Fri, 28 Sep 2018 12:16:05 -0600
Subject: [PATCH 08/19] Move FIXME comment to where the fix should happen

---
 src/coins.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/coins.cpp b/src/coins.cpp
index edc4d2e20..31f2dbab0 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -620,12 +620,12 @@ double CCoinsViewCache::GetPriority(const CTransaction &tx, int nHeight) const
     // cannot apply the priority algorithm used for transparent utxos.  Instead, we just
     // use the maximum priority for all (partially or fully) shielded transactions.
     // (Note that coinbase transactions cannot contain JoinSplits, or Sapling shielded Spends or Outputs.)
-    // FIXME: this logic is partially duplicated between here and CreateNewBlock in miner.cpp.
 
     if (tx.vjoinsplit.size() > 0 || tx.vShieldedSpend.size() > 0 || tx.vShieldedOutput.size() > 0) {
         return MAX_PRIORITY;
     }
 
+    // FIXME: this logic is partially duplicated between here and CreateNewBlock in miner.cpp.
     double dResult = 0.0;
     BOOST_FOREACH(const CTxIn& txin, tx.vin)
     {

From 06f2a8f9b61684fca6b0f2e834fabc6353947c7c Mon Sep 17 00:00:00 2001
From: Jay Graber <jay@z.cash>
Date: Thu, 13 Sep 2018 16:27:15 -0700
Subject: [PATCH 09/19] s/jsoutindex/outindex for sapling outputs

---
 src/wallet/rpcwallet.cpp | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index d93279986..d46ba7b85 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -2473,7 +2473,8 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
             "  {\n"
             "    \"txid\" : \"txid\",          (string) the transaction id \n"
             "    \"jsindex\" : n             (numeric) the joinsplit index\n"
-            "    \"jsoutindex\" : n          (numeric) the output index of the joinsplit\n"
+            "    \"jsoutindex\" (sprout) : n          (numeric) the output index of the joinsplit\n"
+            "    \"outindex\" (sapling) : n          (numeric) the output index\n"
             "    \"confirmations\" : n       (numeric) the number of confirmations\n"
             "    \"spendable\" : true|false  (boolean) true if note can be spent by wallet, false if note has zero confirmations, false if address is watchonly\n"
             "    \"address\" : \"address\",    (string) the shielded address\n"
@@ -2584,6 +2585,25 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
             results.push_back(obj);
         }
         // TODO: Sapling
+        for (UnspentSaplingNoteEntry & entry : saplingEntries) {
+          UniValue obj(UniValue::VOBJ);
+          obj.push_back(Pair("txid", entry.op.hash.ToString()));
+          obj.push_back(Pair("outindex", (int)entry.op.n));
+          obj.push_back(Pair("confirmations", entry.nHeight));
+          libzcash::SaplingIncomingViewingKey ivk;
+          libzcash::SaplingFullViewingKey fvk;
+          pwalletMain->GetSaplingIncomingViewingKey(boost::get<libzcash::SaplingPaymentAddress>(entry.address), ivk);
+          pwalletMain->GetSaplingFullViewingKey(ivk, fvk);
+          bool hasSaplingSpendingKey = pwalletMain->HaveSaplingSpendingKey(fvk);
+          obj.push_back(Pair("spendable", hasSaplingSpendingKey));
+          obj.push_back(Pair("address", EncodePaymentAddress(entry.address)));
+          obj.push_back(Pair("amount", ValueFromAmount(CAmount(entry.note.value())))); // note.value() == plaintext.value()
+          obj.push_back(Pair("memo", HexStr(entry.memo)));
+          if (hasSaplingSpendingKey) {
+              obj.push_back(Pair("change", pwalletMain->IsNoteSaplingChange(nullifierSet, entry.address, entry.op)));
+          }
+          results.push_back(obj);
+        }
     }
 
     return results;

From 66795a408b557be002a14da9ab1782f184b92311 Mon Sep 17 00:00:00 2001
From: Jay Graber <jay@z.cash>
Date: Mon, 27 Aug 2018 18:04:38 -0700
Subject: [PATCH 10/19] z_listunspent sapling support - needs refactor

---
 src/wallet/rpcwallet.cpp | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index d46ba7b85..b0807e1b6 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -2463,7 +2463,7 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
             "1. minconf          (numeric, optional, default=1) The minimum confirmations to filter\n"
             "2. maxconf          (numeric, optional, default=9999999) The maximum confirmations to filter\n"
             "3. includeWatchonly (bool, optional, default=false) Also include watchonly addresses (see 'z_importviewingkey')\n"
-            "4. \"addresses\"      (string) A json array of zaddrs to filter on.  Duplicate addresses not allowed.\n"
+            "4. \"addresses\"      (string) A json array of zaddrs (both Sprout and Sapling) to filter on.  Duplicate addresses not allowed.\n"
             "    [\n"
             "      \"address\"     (string) zaddr\n"
             "      ,...\n"
@@ -2536,12 +2536,25 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
             auto zaddr = DecodePaymentAddress(address);
             if (IsValidPaymentAddress(zaddr)) {
                 // TODO: Add Sapling support. For now, ensure we can freely convert.
-                assert(boost::get<libzcash::SproutPaymentAddress>(&zaddr) != nullptr);
-                libzcash::SproutPaymentAddress addr = boost::get<libzcash::SproutPaymentAddress>(zaddr);
-                if (!fIncludeWatchonly && !pwalletMain->HaveSproutSpendingKey(addr)) {
+                if (boost::get<libzcash::SproutPaymentAddress>(&zaddr) != nullptr){
+                  libzcash::SproutPaymentAddress sproutAddr = boost::get<libzcash::SproutPaymentAddress>(zaddr);
+                  if (!fIncludeWatchonly && !pwalletMain->HaveSproutSpendingKey(sproutAddr)) {
                     throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, spending key for address does not belong to wallet: ") + address);
+                  }
+                  zaddrs.insert(sproutAddr);
+                } else if (boost::get<libzcash::SaplingPaymentAddress>(&zaddr) != nullptr) {
+                  libzcash::SaplingPaymentAddress saplingAddr = boost::get<libzcash::SaplingPaymentAddress>(zaddr);
+                  libzcash::SaplingIncomingViewingKey ivk;
+                  libzcash::SaplingFullViewingKey fvk;
+                  if (!fIncludeWatchonly && 
+                    !pwalletMain->GetSaplingIncomingViewingKey(saplingAddr, ivk) &&
+                    !pwalletMain->GetSaplingFullViewingKey(ivk, fvk) &&
+                    !pwalletMain->HaveSaplingSpendingKey(fvk)) {
+                    throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, spending key for address does not belong to wallet: ") + address);
+                  }
+                  zaddrs.insert(saplingAddr);
                 }
-                zaddrs.insert(addr);
+
             } else {
                 throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, address is not a valid zaddr: ") + address);
             }
@@ -2557,7 +2570,12 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
         // TODO: Add Sapling support
         std::set<libzcash::SproutPaymentAddress> sproutzaddrs = {};
         pwalletMain->GetSproutPaymentAddresses(sproutzaddrs);
+        
+        std::set<libzcash::SaplingPaymentAddress> saplingzaddrs = {};
+        pwalletMain->GetSaplingPaymentAddresses(saplingzaddrs);
+        
         zaddrs.insert(sproutzaddrs.begin(), sproutzaddrs.end());
+        zaddrs.insert(saplingzaddrs.begin(), saplingzaddrs.end());
     }
 
     UniValue results(UniValue::VARR);
@@ -2567,6 +2585,7 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
         std::vector<UnspentSaplingNoteEntry> saplingEntries;
         pwalletMain->GetUnspentFilteredNotes(sproutEntries, saplingEntries, zaddrs, nMinDepth, nMaxDepth, !fIncludeWatchonly);
         std::set<std::pair<PaymentAddress, uint256>> nullifierSet = pwalletMain->GetNullifiersForAddresses(zaddrs);
+        
         for (CUnspentSproutNotePlaintextEntry & entry : sproutEntries) {
             UniValue obj(UniValue::VOBJ);
             obj.push_back(Pair("txid", entry.jsop.hash.ToString()));
@@ -2584,6 +2603,7 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
             }
             results.push_back(obj);
         }
+        
         // TODO: Sapling
         for (UnspentSaplingNoteEntry & entry : saplingEntries) {
           UniValue obj(UniValue::VOBJ);

From 011f9a02ef10eb1429c2aa697e2c4511ef698ee7 Mon Sep 17 00:00:00 2001
From: Jay Graber <jay@z.cash>
Date: Tue, 28 Aug 2018 17:16:04 -0700
Subject: [PATCH 11/19] Add rpc test for sprout txs z_listunspent

---
 qa/pull-tester/rpc-tests.sh      |  1 +
 qa/rpc-tests/wallet_listnotes.py | 84 ++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)
 create mode 100755 qa/rpc-tests/wallet_listnotes.py

diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh
index dde4144c4..50244440e 100755
--- a/qa/pull-tester/rpc-tests.sh
+++ b/qa/pull-tester/rpc-tests.sh
@@ -27,6 +27,7 @@ testScripts=(
     'wallet_1941.py'
     'wallet_addresses.py'
     'wallet_sapling.py'
+    'wallet_listnotes.py'
     'listtransactions.py'
     'mempool_resurrect_test.py'
     'txn_doublespend.py'
diff --git a/qa/rpc-tests/wallet_listnotes.py b/qa/rpc-tests/wallet_listnotes.py
new file mode 100755
index 000000000..f839b1ffd
--- /dev/null
+++ b/qa/rpc-tests/wallet_listnotes.py
@@ -0,0 +1,84 @@
+#!/usr/bin/env python2
+# Copyright (c) 2018 The Zcash developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal, start_nodes, wait_and_assert_operationid_status
+
+from decimal import Decimal
+
+# Test wallet z_listunspent and z_listreceivedbyaddress behaviour across network upgrades
+# TODO: Test z_listreceivedbyaddress
+class WalletListNotes(BitcoinTestFramework):
+
+    def setup_nodes(self):
+        return start_nodes(4, self.options.tmpdir, [[
+            '-nuparams=5ba81b19:202', # Overwinter
+            '-nuparams=76b809bb:204', # Sapling
+        ]] * 4)
+
+    def run_test(self):
+        # Current height = 200 -> Sprout
+        sproutzaddr = self.nodes[0].z_getnewaddress('sprout')
+        saplingzaddr = self.nodes[0].z_getnewaddress('sapling')
+        print "sprout zaddr", sproutzaddr
+        print "saplingzaddr", saplingzaddr
+        
+        # Current height = 201 -> Sprout
+        self.nodes[0].generate(1)
+        self.sync_all()
+        
+        mining_addr = self.nodes[0].listunspent()[0]['address']
+        # Shield coinbase funds 
+        recipients = []
+        recipients.append({"address":sproutzaddr, "amount":Decimal('10.0')-Decimal('0.0001')}) # utxo amount less fee
+        myopid = self.nodes[0].z_sendmany(mining_addr, recipients)
+        txid_1 = wait_and_assert_operationid_status(self.nodes[0], myopid)
+        
+        # No funds in sproutzaddr yet
+        assert_equal(set(self.nodes[0].z_listunspent()), set())
+        
+        print self.nodes[0].z_gettotalbalance() # no private balance displayed bc no confirmations yet
+        
+        # list unspent, allowing 0 confirmations
+        unspent_cb = self.nodes[0].z_listunspent(0)
+        # list unspent, filtering by address
+        unspent_cb_filter = self.nodes[0].z_listunspent(0, 9999, False, [sproutzaddr]) 
+        assert_equal(unspent_cb, unspent_cb_filter)
+        assert_equal(len(unspent_cb), 1)
+        assert_equal(unspent_cb[0]['change'], False)
+        assert_equal(unspent_cb[0]['txid'], txid_1)
+        assert_equal(unspent_cb[0]['spendable'], True)
+        assert_equal(unspent_cb[0]['address'], sproutzaddr)
+        assert_equal(unspent_cb[0]['amount'], Decimal('10.0')-Decimal('0.0001'))
+        
+        # Generate a block to confirm shield coinbase tx        
+        # Current height = 202 -> Overwinter. Default address type is Sprout
+        self.nodes[0].generate(1)
+        self.sync_all()
+        
+        sproutzaddr2 = self.nodes[0].z_getnewaddress('sprout')
+        recipients = [{"address": sproutzaddr2, "amount":Decimal('1.0')-Decimal('0.0001')}]
+        myopid = self.nodes[0].z_sendmany(sproutzaddr, recipients)
+        txid_2 = wait_and_assert_operationid_status(self.nodes[0], myopid)
+        
+        # list unspent, allowing 0conf txs
+        unspent_tx = self.nodes[0].z_listunspent(0)
+        unspent_tx_filter = self.nodes[0].z_listunspent(0, 9999, False, [sproutzaddr2]) 
+        assert_equal(len(unspent_tx), 2)
+        assert_equal(unspent_tx[0]['change'], False)
+        assert_equal(unspent_tx[0]['txid'], txid_2)
+        assert_equal(unspent_tx[0]['spendable'], True)
+        assert_equal(unspent_tx[0]['address'], sproutzaddr2)
+        assert_equal(unspent_tx[0]['amount'], Decimal('1.0')-Decimal('0.0001'))
+        # TODO: test change
+        
+        # No funds in saplingzaddr yet
+        assert_equal(set(self.nodes[0].z_listunspent(0, 9999, False, [saplingzaddr])), set())
+        
+        # Current height = 204 -> Sapling
+        # self.nodes[0].generate(2)
+
+if __name__ == '__main__':
+    WalletListNotes().main()
\ No newline at end of file

From cd1c6e3767d3524bbd2609976eae8d16bd987c6a Mon Sep 17 00:00:00 2001
From: Jay Graber <jay@z.cash>
Date: Thu, 6 Sep 2018 13:59:45 -0700
Subject: [PATCH 12/19] Modify comments

---
 src/wallet/rpcwallet.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index b0807e1b6..02c899187 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -2535,7 +2535,7 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
             string address = o.get_str();
             auto zaddr = DecodePaymentAddress(address);
             if (IsValidPaymentAddress(zaddr)) {
-                // TODO: Add Sapling support. For now, ensure we can freely convert.
+                // TODO: Add visitor to handle support for Sprout and Sapling addrs.
                 if (boost::get<libzcash::SproutPaymentAddress>(&zaddr) != nullptr){
                   libzcash::SproutPaymentAddress sproutAddr = boost::get<libzcash::SproutPaymentAddress>(zaddr);
                   if (!fIncludeWatchonly && !pwalletMain->HaveSproutSpendingKey(sproutAddr)) {
@@ -2567,10 +2567,10 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
     }
     else {
         // User did not provide zaddrs, so use default i.e. all addresses
-        // TODO: Add Sapling support
         std::set<libzcash::SproutPaymentAddress> sproutzaddrs = {};
         pwalletMain->GetSproutPaymentAddresses(sproutzaddrs);
         
+        // Sapling support
         std::set<libzcash::SaplingPaymentAddress> saplingzaddrs = {};
         pwalletMain->GetSaplingPaymentAddresses(saplingzaddrs);
         

From 27b3cce94f92e2842c4fcab576c3059e9ee2aea8 Mon Sep 17 00:00:00 2001
From: Jay Graber <jay@z.cash>
Date: Thu, 6 Sep 2018 15:51:02 -0700
Subject: [PATCH 13/19] Modify GetNullifiersForAddresses for Sapling

---
 src/wallet/wallet.cpp | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index d838f485e..d4ef17616 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -548,6 +548,12 @@ std::set<std::pair<libzcash::PaymentAddress, uint256>> CWallet::GetNullifiersFor
                 }
             }
         }
+        // Sapling
+        for (const auto & noteDataPair : txPair.second.mapSaplingNoteData) {
+            if (noteDataPair.second.nullifier && ivkMap.count(noteDataPair.second.ivk)) {
+                nullifierSet.insert(std::make_pair(ivkMap[noteDataPair.second.ivk], noteDataPair.second.nullifier.get()));
+            }
+        }
     }
     return nullifierSet;
 }

From d7d6480ce35893de1342f050f793cba81c853692 Mon Sep 17 00:00:00 2001
From: Larry Ruane <larry@z.cash>
Date: Tue, 11 Sep 2018 14:51:38 -0600
Subject: [PATCH 14/19] z_listunspent rpc unit test: add testing for Sapling

---
 qa/rpc-tests/wallet_listnotes.py | 155 ++++++++++++++++++++++++-------
 1 file changed, 120 insertions(+), 35 deletions(-)

diff --git a/qa/rpc-tests/wallet_listnotes.py b/qa/rpc-tests/wallet_listnotes.py
index f839b1ffd..32997df43 100755
--- a/qa/rpc-tests/wallet_listnotes.py
+++ b/qa/rpc-tests/wallet_listnotes.py
@@ -20,65 +20,150 @@ class WalletListNotes(BitcoinTestFramework):
 
     def run_test(self):
         # Current height = 200 -> Sprout
+        assert_equal(200, self.nodes[0].getblockcount())
         sproutzaddr = self.nodes[0].z_getnewaddress('sprout')
+
+        # test that we can create a sapling zaddr before sapling activates
         saplingzaddr = self.nodes[0].z_getnewaddress('sapling')
-        print "sprout zaddr", sproutzaddr
-        print "saplingzaddr", saplingzaddr
+
+        # we've got lots of coinbase (taddr) but no shielded funds yet
+        assert_equal(0, Decimal(self.nodes[0].z_gettotalbalance()['private']))
         
-        # Current height = 201 -> Sprout
+        # Set current height to 201 -> Sprout
         self.nodes[0].generate(1)
         self.sync_all()
-        
+        assert_equal(201, self.nodes[0].getblockcount())
+
         mining_addr = self.nodes[0].listunspent()[0]['address']
-        # Shield coinbase funds 
-        recipients = []
-        recipients.append({"address":sproutzaddr, "amount":Decimal('10.0')-Decimal('0.0001')}) # utxo amount less fee
+
+        # Shield coinbase funds (must be a multiple of 10, no change allowed pre-sapling)
+        receive_amount_10 = Decimal('10.0') - Decimal('0.0001')
+        recipients = [{"address":sproutzaddr, "amount":receive_amount_10}]
         myopid = self.nodes[0].z_sendmany(mining_addr, recipients)
         txid_1 = wait_and_assert_operationid_status(self.nodes[0], myopid)
+        self.sync_all()
         
-        # No funds in sproutzaddr yet
-        assert_equal(set(self.nodes[0].z_listunspent()), set())
+        # No funds (with (default) one or more confirmations) in sproutzaddr yet
+        assert_equal(0, len(self.nodes[0].z_listunspent()))
+        assert_equal(0, len(self.nodes[0].z_listunspent(1)))
         
-        print self.nodes[0].z_gettotalbalance() # no private balance displayed bc no confirmations yet
+        # no private balance because no confirmations yet
+        assert_equal(0, Decimal(self.nodes[0].z_gettotalbalance()['private']))
         
-        # list unspent, allowing 0 confirmations
+        # list private unspent, this time allowing 0 confirmations
         unspent_cb = self.nodes[0].z_listunspent(0)
-        # list unspent, filtering by address
-        unspent_cb_filter = self.nodes[0].z_listunspent(0, 9999, False, [sproutzaddr]) 
+        assert_equal(1, len(unspent_cb))
+        assert_equal(False,             unspent_cb[0]['change'])
+        assert_equal(txid_1,            unspent_cb[0]['txid'])
+        assert_equal(True,              unspent_cb[0]['spendable'])
+        assert_equal(sproutzaddr,       unspent_cb[0]['address'])
+        assert_equal(receive_amount_10, unspent_cb[0]['amount'])
+
+        # list unspent, filtering by address, should produce same result
+        unspent_cb_filter = self.nodes[0].z_listunspent(0, 9999, False, [sproutzaddr])
         assert_equal(unspent_cb, unspent_cb_filter)
-        assert_equal(len(unspent_cb), 1)
-        assert_equal(unspent_cb[0]['change'], False)
-        assert_equal(unspent_cb[0]['txid'], txid_1)
-        assert_equal(unspent_cb[0]['spendable'], True)
-        assert_equal(unspent_cb[0]['address'], sproutzaddr)
-        assert_equal(unspent_cb[0]['amount'], Decimal('10.0')-Decimal('0.0001'))
         
-        # Generate a block to confirm shield coinbase tx        
-        # Current height = 202 -> Overwinter. Default address type is Sprout
+        # Generate a block to confirm shield coinbase tx
         self.nodes[0].generate(1)
         self.sync_all()
         
-        sproutzaddr2 = self.nodes[0].z_getnewaddress('sprout')
-        recipients = [{"address": sproutzaddr2, "amount":Decimal('1.0')-Decimal('0.0001')}]
+        # Current height = 202 -> Overwinter. Default address type remains Sprout
+        assert_equal(202, self.nodes[0].getblockcount())
+
+        # Send 1.0 (actually 0.9999) from sproutzaddr to a new zaddr
+        sproutzaddr2 = self.nodes[0].z_getnewaddress()
+        receive_amount_1 = Decimal('1.0') - Decimal('0.0001')
+        change_amount_9 = receive_amount_10 - Decimal('1.0')
+        assert_equal('sprout', self.nodes[0].z_validateaddress(sproutzaddr2)['type'])
+        recipients = [{"address": sproutzaddr2, "amount":receive_amount_1}]
         myopid = self.nodes[0].z_sendmany(sproutzaddr, recipients)
         txid_2 = wait_and_assert_operationid_status(self.nodes[0], myopid)
+        self.sync_all()
         
         # list unspent, allowing 0conf txs
         unspent_tx = self.nodes[0].z_listunspent(0)
-        unspent_tx_filter = self.nodes[0].z_listunspent(0, 9999, False, [sproutzaddr2]) 
         assert_equal(len(unspent_tx), 2)
-        assert_equal(unspent_tx[0]['change'], False)
-        assert_equal(unspent_tx[0]['txid'], txid_2)
-        assert_equal(unspent_tx[0]['spendable'], True)
-        assert_equal(unspent_tx[0]['address'], sproutzaddr2)
-        assert_equal(unspent_tx[0]['amount'], Decimal('1.0')-Decimal('0.0001'))
-        # TODO: test change
+        # sort low-to-high by amount (order of returned entries is not guaranteed)
+        unspent_tx = sorted(unspent_tx, key=lambda k: k['amount'])
+        assert_equal(False,             unspent_tx[0]['change'])
+        assert_equal(txid_2,            unspent_tx[0]['txid'])
+        assert_equal(True,              unspent_tx[0]['spendable'])
+        assert_equal(sproutzaddr2,      unspent_tx[0]['address'])
+        assert_equal(receive_amount_1,  unspent_tx[0]['amount'])
+
+        assert_equal(True,              unspent_tx[1]['change'])
+        assert_equal(txid_2,            unspent_tx[1]['txid'])
+        assert_equal(True,              unspent_tx[1]['spendable'])
+        assert_equal(sproutzaddr,       unspent_tx[1]['address'])
+        assert_equal(change_amount_9,   unspent_tx[1]['amount'])
+
+        unspent_tx_filter = self.nodes[0].z_listunspent(0, 9999, False, [sproutzaddr2])
+        assert_equal(1, len(unspent_tx_filter))
+        assert_equal(unspent_tx[0], unspent_tx_filter[0])
+
+        unspent_tx_filter = self.nodes[0].z_listunspent(0, 9999, False, [sproutzaddr])
+        assert_equal(1, len(unspent_tx_filter))
+        assert_equal(unspent_tx[1], unspent_tx_filter[0])
+        
+        # Set current height to 204 -> Sapling
+        self.nodes[0].generate(2)
+        self.sync_all()
+        assert_equal(204, self.nodes[0].getblockcount())
         
         # No funds in saplingzaddr yet
-        assert_equal(set(self.nodes[0].z_listunspent(0, 9999, False, [saplingzaddr])), set())
-        
-        # Current height = 204 -> Sapling
-        # self.nodes[0].generate(2)
+        assert_equal(0, len(self.nodes[0].z_listunspent(0, 9999, False, [saplingzaddr])))
+
+        # Send 0.9999 to our sapling zaddr
+        # (sending from a sprout zaddr to a sapling zaddr is disallowed,
+        # so send from coin base)
+        receive_amount_2 = Decimal('2.0') - Decimal('0.0001')
+        recipients = [{"address": saplingzaddr, "amount":receive_amount_2}]
+        myopid = self.nodes[0].z_sendmany(mining_addr, recipients)
+        txid_3 = wait_and_assert_operationid_status(self.nodes[0], myopid)
+        self.sync_all()
+        unspent_tx = self.nodes[0].z_listunspent(0)
+        assert_equal(3, len(unspent_tx))
+
+        # low-to-high in amount
+        unspent_tx = sorted(unspent_tx, key=lambda k: k['amount'])
+
+        assert_equal(False,             unspent_tx[0]['change'])
+        assert_equal(txid_2,            unspent_tx[0]['txid'])
+        assert_equal(True,              unspent_tx[0]['spendable'])
+        assert_equal(sproutzaddr2,      unspent_tx[0]['address'])
+        assert_equal(receive_amount_1,  unspent_tx[0]['amount'])
+
+        assert_equal(False,             unspent_tx[1]['change'])
+        assert_equal(txid_3,            unspent_tx[1]['txid'])
+        assert_equal(True,              unspent_tx[1]['spendable'])
+        assert_equal(saplingzaddr,      unspent_tx[1]['address'])
+        assert_equal(receive_amount_2,  unspent_tx[1]['amount'])
+
+        assert_equal(True,              unspent_tx[2]['change'])
+        assert_equal(txid_2,            unspent_tx[2]['txid'])
+        assert_equal(True,              unspent_tx[2]['spendable'])
+        assert_equal(sproutzaddr,       unspent_tx[2]['address'])
+        assert_equal(change_amount_9,   unspent_tx[2]['amount'])
+
+        unspent_tx_filter = self.nodes[0].z_listunspent(0, 9999, False, [saplingzaddr])
+        assert_equal(1, len(unspent_tx_filter))
+        assert_equal(unspent_tx[1], unspent_tx_filter[0])
+
+        # test that pre- and post-sapling can be filtered in a single call
+        unspent_tx_filter = self.nodes[0].z_listunspent(0, 9999, False,
+            [sproutzaddr, saplingzaddr])
+        assert_equal(2, len(unspent_tx_filter))
+        unspent_tx_filter = sorted(unspent_tx_filter, key=lambda k: k['amount'])
+        assert_equal(unspent_tx[1], unspent_tx_filter[0])
+        assert_equal(unspent_tx[2], unspent_tx_filter[1])
+
+        # so far, this node has no watchonly addresses, so results are the same
+        unspent_tx_watchonly = self.nodes[0].z_listunspent(0, 9999, True)
+        unspent_tx_watchonly = sorted(unspent_tx_watchonly, key=lambda k: k['amount'])
+        assert_equal(unspent_tx, unspent_tx_watchonly)
+
+        # TODO: use z_exportviewingkey, z_importviewingkey to test includeWatchonly
+        # but this requires Sapling support for those RPCs
 
 if __name__ == '__main__':
-    WalletListNotes().main()
\ No newline at end of file
+    WalletListNotes().main()

From c0f7e4059db2221473ecb4f94e6fd5966260bb5c Mon Sep 17 00:00:00 2001
From: Simon <simon@bitcartel.com>
Date: Fri, 28 Sep 2018 22:08:07 -0700
Subject: [PATCH 15/19] Fix rebasing of CWallet::GetNullifiersForAddresses

---
 src/wallet/wallet.cpp | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index d4ef17616..d838f485e 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -548,12 +548,6 @@ std::set<std::pair<libzcash::PaymentAddress, uint256>> CWallet::GetNullifiersFor
                 }
             }
         }
-        // Sapling
-        for (const auto & noteDataPair : txPair.second.mapSaplingNoteData) {
-            if (noteDataPair.second.nullifier && ivkMap.count(noteDataPair.second.ivk)) {
-                nullifierSet.insert(std::make_pair(ivkMap[noteDataPair.second.ivk], noteDataPair.second.nullifier.get()));
-            }
-        }
     }
     return nullifierSet;
 }

From 5f57babd08ac29db317c4e8352ee2997c03d1bc5 Mon Sep 17 00:00:00 2001
From: Simon <simon@bitcartel.com>
Date: Fri, 28 Sep 2018 22:11:05 -0700
Subject: [PATCH 16/19] Cleanup to address review comments.

---
 qa/rpc-tests/wallet_listnotes.py |  3 +-
 src/wallet/rpcwallet.cpp         | 60 ++++++++++++--------------------
 2 files changed, 23 insertions(+), 40 deletions(-)

diff --git a/qa/rpc-tests/wallet_listnotes.py b/qa/rpc-tests/wallet_listnotes.py
index 32997df43..5cd89c661 100755
--- a/qa/rpc-tests/wallet_listnotes.py
+++ b/qa/rpc-tests/wallet_listnotes.py
@@ -8,8 +8,7 @@ from test_framework.util import assert_equal, start_nodes, wait_and_assert_opera
 
 from decimal import Decimal
 
-# Test wallet z_listunspent and z_listreceivedbyaddress behaviour across network upgrades
-# TODO: Test z_listreceivedbyaddress
+# Test wallet z_listunspent behaviour across network upgrades
 class WalletListNotes(BitcoinTestFramework):
 
     def setup_nodes(self):
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 02c899187..78226452c 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -2458,7 +2458,8 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
             "Optionally filter to only include notes sent to specified addresses.\n"
             "When minconf is 0, unspent notes with zero confirmations are returned, even though they are not immediately spendable.\n"
             "Results are an array of Objects, each of which has:\n"
-            "{txid, jsindex, jsoutindex, confirmations, address, amount, memo}\n"
+            "{txid, jsindex, jsoutindex, confirmations, address, amount, memo} (Sprout)\n"
+            "{txid, outindex, confirmations, address, amount, memo} (Sapling)\n"
             "\nArguments:\n"
             "1. minconf          (numeric, optional, default=1) The minimum confirmations to filter\n"
             "2. maxconf          (numeric, optional, default=9999999) The maximum confirmations to filter\n"
@@ -2535,26 +2536,10 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
             string address = o.get_str();
             auto zaddr = DecodePaymentAddress(address);
             if (IsValidPaymentAddress(zaddr)) {
-                // TODO: Add visitor to handle support for Sprout and Sapling addrs.
-                if (boost::get<libzcash::SproutPaymentAddress>(&zaddr) != nullptr){
-                  libzcash::SproutPaymentAddress sproutAddr = boost::get<libzcash::SproutPaymentAddress>(zaddr);
-                  if (!fIncludeWatchonly && !pwalletMain->HaveSproutSpendingKey(sproutAddr)) {
-                    throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, spending key for address does not belong to wallet: ") + address);
-                  }
-                  zaddrs.insert(sproutAddr);
-                } else if (boost::get<libzcash::SaplingPaymentAddress>(&zaddr) != nullptr) {
-                  libzcash::SaplingPaymentAddress saplingAddr = boost::get<libzcash::SaplingPaymentAddress>(zaddr);
-                  libzcash::SaplingIncomingViewingKey ivk;
-                  libzcash::SaplingFullViewingKey fvk;
-                  if (!fIncludeWatchonly && 
-                    !pwalletMain->GetSaplingIncomingViewingKey(saplingAddr, ivk) &&
-                    !pwalletMain->GetSaplingFullViewingKey(ivk, fvk) &&
-                    !pwalletMain->HaveSaplingSpendingKey(fvk)) {
-                    throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, spending key for address does not belong to wallet: ") + address);
-                  }
-                  zaddrs.insert(saplingAddr);
+                auto hasSpendingKey = boost::apply_visitor(HaveSpendingKeyForPaymentAddress(pwalletMain), zaddr);
+                if (hasSpendingKey) {
+                    zaddrs.insert(zaddr);
                 }
-
             } else {
                 throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, address is not a valid zaddr: ") + address);
             }
@@ -2604,25 +2589,24 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
             results.push_back(obj);
         }
         
-        // TODO: Sapling
         for (UnspentSaplingNoteEntry & entry : saplingEntries) {
-          UniValue obj(UniValue::VOBJ);
-          obj.push_back(Pair("txid", entry.op.hash.ToString()));
-          obj.push_back(Pair("outindex", (int)entry.op.n));
-          obj.push_back(Pair("confirmations", entry.nHeight));
-          libzcash::SaplingIncomingViewingKey ivk;
-          libzcash::SaplingFullViewingKey fvk;
-          pwalletMain->GetSaplingIncomingViewingKey(boost::get<libzcash::SaplingPaymentAddress>(entry.address), ivk);
-          pwalletMain->GetSaplingFullViewingKey(ivk, fvk);
-          bool hasSaplingSpendingKey = pwalletMain->HaveSaplingSpendingKey(fvk);
-          obj.push_back(Pair("spendable", hasSaplingSpendingKey));
-          obj.push_back(Pair("address", EncodePaymentAddress(entry.address)));
-          obj.push_back(Pair("amount", ValueFromAmount(CAmount(entry.note.value())))); // note.value() == plaintext.value()
-          obj.push_back(Pair("memo", HexStr(entry.memo)));
-          if (hasSaplingSpendingKey) {
-              obj.push_back(Pair("change", pwalletMain->IsNoteSaplingChange(nullifierSet, entry.address, entry.op)));
-          }
-          results.push_back(obj);
+            UniValue obj(UniValue::VOBJ);
+            obj.push_back(Pair("txid", entry.op.hash.ToString()));
+            obj.push_back(Pair("outindex", (int)entry.op.n));
+            obj.push_back(Pair("confirmations", entry.nHeight));
+            libzcash::SaplingIncomingViewingKey ivk;
+            libzcash::SaplingFullViewingKey fvk;
+            pwalletMain->GetSaplingIncomingViewingKey(boost::get<libzcash::SaplingPaymentAddress>(entry.address), ivk);
+            pwalletMain->GetSaplingFullViewingKey(ivk, fvk);
+            bool hasSaplingSpendingKey = pwalletMain->HaveSaplingSpendingKey(fvk);
+            obj.push_back(Pair("spendable", hasSaplingSpendingKey));
+            obj.push_back(Pair("address", EncodePaymentAddress(entry.address)));
+            obj.push_back(Pair("amount", ValueFromAmount(CAmount(entry.note.value())))); // note.value() is equivalent to plaintext.value()
+            obj.push_back(Pair("memo", HexStr(entry.memo)));
+            if (hasSaplingSpendingKey) {
+                obj.push_back(Pair("change", pwalletMain->IsNoteSaplingChange(nullifierSet, entry.address, entry.op)));
+            }
+            results.push_back(obj);
         }
     }
 

From b7549f2aec90b44367169b4e0b113f2f27c23bf1 Mon Sep 17 00:00:00 2001
From: Simon <simon@bitcartel.com>
Date: Sun, 30 Sep 2018 17:23:53 -0700
Subject: [PATCH 17/19] Add test that Sapling shielded transactions have
 MAX_PRIORITY

---
 qa/rpc-tests/wallet_sapling.py | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/qa/rpc-tests/wallet_sapling.py b/qa/rpc-tests/wallet_sapling.py
index 0ce1c734f..bba426868 100755
--- a/qa/rpc-tests/wallet_sapling.py
+++ b/qa/rpc-tests/wallet_sapling.py
@@ -53,9 +53,14 @@ class WalletSaplingTest(BitcoinTestFramework):
         recipients = []
         recipients.append({"address": saplingAddr0, "amount": Decimal('20')})
         myopid = self.nodes[0].z_sendmany(taddr0, recipients, 1, 0)
-        wait_and_assert_operationid_status(self.nodes[0], myopid)
+        mytxid = wait_and_assert_operationid_status(self.nodes[0], myopid)
 
         self.sync_all()
+
+        # Verify priority of tx is MAX_PRIORITY, defined as 1E+16 (10000000000000000)
+        mempool = self.nodes[0].getrawmempool(True)
+        assert(Decimal(mempool[mytxid]['startingpriority']) == Decimal('1E+16'))
+
         self.nodes[2].generate(1)
         self.sync_all()
 
@@ -70,9 +75,14 @@ class WalletSaplingTest(BitcoinTestFramework):
         recipients = []
         recipients.append({"address": saplingAddr1, "amount": Decimal('15')})
         myopid = self.nodes[0].z_sendmany(saplingAddr0, recipients, 1, 0)
-        wait_and_assert_operationid_status(self.nodes[0], myopid)
+        mytxid = wait_and_assert_operationid_status(self.nodes[0], myopid)
 
         self.sync_all()
+
+        # Verify priority of tx is MAX_PRIORITY, defined as 1E+16 (10000000000000000)
+        mempool = self.nodes[0].getrawmempool(True)
+        assert(Decimal(mempool[mytxid]['startingpriority']) == Decimal('1E+16'))
+
         self.nodes[2].generate(1)
         self.sync_all()
 
@@ -92,6 +102,11 @@ class WalletSaplingTest(BitcoinTestFramework):
         mytxid = wait_and_assert_operationid_status(self.nodes[1], myopid)
 
         self.sync_all()
+
+        # Verify priority of tx is MAX_PRIORITY, defined as 1E+16 (10000000000000000)
+        mempool = self.nodes[1].getrawmempool(True)
+        assert(Decimal(mempool[mytxid]['startingpriority']) == Decimal('1E+16'))
+
         self.nodes[2].generate(1)
         self.sync_all()
 

From a4ecd0fa72684f9cf44145ad6a9d8929d835e4f8 Mon Sep 17 00:00:00 2001
From: Eirik Ogilvie-Wigley <eirik@z.cash>
Date: Mon, 1 Oct 2018 09:34:25 -0600
Subject: [PATCH 18/19] Add newly discovered sapling addresses to the wallet

---
 src/keystore.cpp                 | 18 +++++++++++++++---
 src/keystore.h                   |  6 ++++++
 src/wallet/gtest/test_wallet.cpp | 18 +++++++++---------
 src/wallet/wallet.cpp            | 18 +++++++++++++++---
 src/wallet/wallet.h              |  2 +-
 5 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/src/keystore.cpp b/src/keystore.cpp
index be5e0b82f..50766518f 100644
--- a/src/keystore.cpp
+++ b/src/keystore.cpp
@@ -156,9 +156,21 @@ bool CBasicKeyStore::AddSaplingFullViewingKey(
     LOCK(cs_SpendingKeyStore);
     auto ivk = fvk.in_viewing_key();
     mapSaplingFullViewingKeys[ivk] = fvk;
-    
-    // Add defaultAddr -> SaplingIncomingViewing to SaplingIncomingViewingKeyMap
-    mapSaplingIncomingViewingKeys[defaultAddr] = ivk;
+
+    return AddSaplingIncomingViewingKey(ivk, defaultAddr);
+}
+
+// This function updates the wallet's internal address->ivk map. 
+// If we add an address that is already in the map, the map will
+// remain unchanged as each address only has one ivk.
+bool CBasicKeyStore::AddSaplingIncomingViewingKey(
+    const libzcash::SaplingIncomingViewingKey &ivk,
+    const libzcash::SaplingPaymentAddress &addr)
+{
+    LOCK(cs_SpendingKeyStore);
+
+    // Add addr -> SaplingIncomingViewing to SaplingIncomingViewingKeyMap
+    mapSaplingIncomingViewingKeys[addr] = ivk;
 
     return true;
 }
diff --git a/src/keystore.h b/src/keystore.h
index 5b064f27e..2cf34ea2c 100644
--- a/src/keystore.h
+++ b/src/keystore.h
@@ -82,6 +82,9 @@ public:
         libzcash::SaplingFullViewingKey& fvkOut) const =0;
     
     //! Sapling incoming viewing keys 
+    virtual bool AddSaplingIncomingViewingKey(
+        const libzcash::SaplingIncomingViewingKey &ivk,
+        const libzcash::SaplingPaymentAddress &addr) =0;
     virtual bool HaveSaplingIncomingViewingKey(const libzcash::SaplingPaymentAddress &addr) const =0;
     virtual bool GetSaplingIncomingViewingKey(
         const libzcash::SaplingPaymentAddress &addr, 
@@ -269,6 +272,9 @@ public:
         const libzcash::SaplingIncomingViewingKey &ivk, 
         libzcash::SaplingFullViewingKey& fvkOut) const;
     
+    virtual bool AddSaplingIncomingViewingKey(
+        const libzcash::SaplingIncomingViewingKey &ivk,
+        const libzcash::SaplingPaymentAddress &addr);
     virtual bool HaveSaplingIncomingViewingKey(const libzcash::SaplingPaymentAddress &addr) const;
     virtual bool GetSaplingIncomingViewingKey(
         const libzcash::SaplingPaymentAddress &addr, 
diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp
index fa066061d..f4b9ddc55 100644
--- a/src/wallet/gtest/test_wallet.cpp
+++ b/src/wallet/gtest/test_wallet.cpp
@@ -512,13 +512,13 @@ TEST(WalletTests, FindMySaplingNotes) {
     // No Sapling notes can be found in tx which does not belong to the wallet
     CWalletTx wtx {&wallet, tx};
     ASSERT_FALSE(wallet.HaveSaplingSpendingKey(fvk));
-    auto noteMap = wallet.FindMySaplingNotes(wtx);
+    auto noteMap = wallet.FindMySaplingNotes(wtx).first;
     EXPECT_EQ(0, noteMap.size());
 
     // Add spending key to wallet, so Sapling notes can be found
     ASSERT_TRUE(wallet.AddSaplingZKey(sk, pk));
     ASSERT_TRUE(wallet.HaveSaplingSpendingKey(fvk));
-    noteMap = wallet.FindMySaplingNotes(wtx);
+    noteMap = wallet.FindMySaplingNotes(wtx).first;
     EXPECT_EQ(2, noteMap.size());
 
     // Revert to default
@@ -664,7 +664,7 @@ TEST(WalletTests, GetConflictedSaplingNotes) {
     EXPECT_EQ(0, chainActive.Height());
 
     // Simulate SyncTransaction which calls AddToWalletIfInvolvingMe
-    auto saplingNoteData = wallet.FindMySaplingNotes(wtx);
+    auto saplingNoteData = wallet.FindMySaplingNotes(wtx).first;
     ASSERT_TRUE(saplingNoteData.size() > 0);
     wtx.SetSaplingNoteData(saplingNoteData);
     wtx.SetMerkleBranch(block);
@@ -938,7 +938,7 @@ TEST(WalletTests, NavigateFromSaplingNullifierToNote) {
 
     // Simulate SyncTransaction which calls AddToWalletIfInvolvingMe
     wtx.SetMerkleBranch(block);
-    auto saplingNoteData = wallet.FindMySaplingNotes(wtx);
+    auto saplingNoteData = wallet.FindMySaplingNotes(wtx).first;
     ASSERT_TRUE(saplingNoteData.size() > 0);
     wtx.SetSaplingNoteData(saplingNoteData);
     wallet.AddToWallet(wtx, true, NULL);
@@ -1064,7 +1064,7 @@ TEST(WalletTests, SpentSaplingNoteIsFromMe) {
     EXPECT_TRUE(chainActive.Contains(&fakeIndex));
     EXPECT_EQ(0, chainActive.Height());
 
-    auto saplingNoteData = wallet.FindMySaplingNotes(wtx);
+    auto saplingNoteData = wallet.FindMySaplingNotes(wtx).first;
     ASSERT_TRUE(saplingNoteData.size() > 0);
     wtx.SetSaplingNoteData(saplingNoteData);
     wtx.SetMerkleBranch(block);
@@ -1141,7 +1141,7 @@ TEST(WalletTests, SpentSaplingNoteIsFromMe) {
     EXPECT_TRUE(chainActive.Contains(&fakeIndex2));
     EXPECT_EQ(1, chainActive.Height());
 
-    auto saplingNoteData2 = wallet.FindMySaplingNotes(wtx2);
+    auto saplingNoteData2 = wallet.FindMySaplingNotes(wtx2).first;
     ASSERT_TRUE(saplingNoteData2.size() > 0);
     wtx2.SetSaplingNoteData(saplingNoteData2);
     wtx2.SetMerkleBranch(block2);
@@ -1769,7 +1769,7 @@ TEST(WalletTests, UpdatedSaplingNoteData) {
     EXPECT_EQ(0, chainActive.Height());
 
     // Simulate SyncTransaction which calls AddToWalletIfInvolvingMe
-    auto saplingNoteData = wallet.FindMySaplingNotes(wtx);
+    auto saplingNoteData = wallet.FindMySaplingNotes(wtx).first;
     ASSERT_TRUE(saplingNoteData.size() == 1); // wallet only has key for change output
     wtx.SetSaplingNoteData(saplingNoteData);
     wtx.SetMerkleBranch(block);
@@ -1787,7 +1787,7 @@ TEST(WalletTests, UpdatedSaplingNoteData) {
     ASSERT_TRUE(wallet.AddSaplingZKey(sk2, pk2));
     ASSERT_TRUE(wallet.HaveSaplingSpendingKey(fvk2));
     CWalletTx wtx2 = wtx;
-    auto saplingNoteData2 = wallet.FindMySaplingNotes(wtx2);
+    auto saplingNoteData2 = wallet.FindMySaplingNotes(wtx2).first;
     ASSERT_TRUE(saplingNoteData2.size() == 2);
     wtx2.SetSaplingNoteData(saplingNoteData2);
 
@@ -1923,7 +1923,7 @@ TEST(WalletTests, MarkAffectedSaplingTransactionsDirty) {
     EXPECT_EQ(0, chainActive.Height());
 
     // Simulate SyncTransaction which calls AddToWalletIfInvolvingMe
-    auto saplingNoteData = wallet.FindMySaplingNotes(wtx);
+    auto saplingNoteData = wallet.FindMySaplingNotes(wtx).first;
     ASSERT_TRUE(saplingNoteData.size() > 0);
     wtx.SetSaplingNoteData(saplingNoteData);
     wtx.SetMerkleBranch(block);
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 2093fadb9..72872f5e1 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1483,7 +1483,14 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pbl
         bool fExisted = mapWallet.count(tx.GetHash()) != 0;
         if (fExisted && !fUpdate) return false;
         auto sproutNoteData = FindMySproutNotes(tx);
-        auto saplingNoteData = FindMySaplingNotes(tx);
+        auto saplingNoteDataAndAddressesToAdd = FindMySaplingNotes(tx);
+        auto saplingNoteData = saplingNoteDataAndAddressesToAdd.first;
+        auto addressesToAdd = saplingNoteDataAndAddressesToAdd.second;
+        for (const auto &addressToAdd : addressesToAdd) {
+            if (!AddSaplingIncomingViewingKey(addressToAdd.second, addressToAdd.first)) {
+                return false;
+            }
+        }
         if (fExisted || IsMine(tx) || IsFromMe(tx) || sproutNoteData.size() > 0 || saplingNoteData.size() > 0)
         {
             CWalletTx wtx(this,tx);
@@ -1644,12 +1651,13 @@ mapSproutNoteData_t CWallet::FindMySproutNotes(const CTransaction &tx) const
  * the result of FindMySaplingNotes (for the addresses available at the time) will
  * already have been cached in CWalletTx.mapSaplingNoteData.
  */
-mapSaplingNoteData_t CWallet::FindMySaplingNotes(const CTransaction &tx) const
+std::pair<mapSaplingNoteData_t, SaplingIncomingViewingKeyMap> CWallet::FindMySaplingNotes(const CTransaction &tx) const
 {
     LOCK(cs_SpendingKeyStore);
     uint256 hash = tx.GetHash();
 
     mapSaplingNoteData_t noteData;
+    SaplingIncomingViewingKeyMap viewingKeysToAdd;
 
     // Protocol Spec: 4.19 Block Chain Scanning (Sapling)
     for (uint32_t i = 0; i < tx.vShieldedOutput.size(); ++i) {
@@ -1660,6 +1668,10 @@ mapSaplingNoteData_t CWallet::FindMySaplingNotes(const CTransaction &tx) const
             if (!result) {
                 continue;
             }
+            auto address = ivk.address(result.get().d);
+            if (address && mapSaplingIncomingViewingKeys.count(address.get()) == 0) {
+                viewingKeysToAdd[address.get()] = ivk;
+            }
             // We don't cache the nullifier here as computing it requires knowledge of the note position
             // in the commitment tree, which can only be determined when the transaction has been mined.
             SaplingOutPoint op {hash, i};
@@ -1670,7 +1682,7 @@ mapSaplingNoteData_t CWallet::FindMySaplingNotes(const CTransaction &tx) const
         }
     }
 
-    return noteData;
+    return std::make_pair(noteData, viewingKeysToAdd);
 }
 
 bool CWallet::IsSproutNullifierFromMe(const uint256& nullifier) const
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 1deb9b3aa..770650d48 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1132,7 +1132,7 @@ public:
         const uint256& hSig,
         uint8_t n) const;
     mapSproutNoteData_t FindMySproutNotes(const CTransaction& tx) const;
-    mapSaplingNoteData_t FindMySaplingNotes(const CTransaction& tx) const;
+    std::pair<mapSaplingNoteData_t, SaplingIncomingViewingKeyMap> FindMySaplingNotes(const CTransaction& tx) const;
     bool IsSproutNullifierFromMe(const uint256& nullifier) const;
     bool IsSaplingNullifierFromMe(const uint256& nullifier) const;
 

From 27a6a99cb05127f8f30df222b5bd1e11f853fcdb Mon Sep 17 00:00:00 2001
From: Larry Ruane <larry@z.cash>
Date: Thu, 4 Oct 2018 12:26:36 -0600
Subject: [PATCH 19/19] fix z_listunspent includeWatchonly logic

---
 src/wallet/rpcwallet.cpp | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 78226452c..9d8ae70a6 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -2535,14 +2535,14 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
             }
             string address = o.get_str();
             auto zaddr = DecodePaymentAddress(address);
-            if (IsValidPaymentAddress(zaddr)) {
-                auto hasSpendingKey = boost::apply_visitor(HaveSpendingKeyForPaymentAddress(pwalletMain), zaddr);
-                if (hasSpendingKey) {
-                    zaddrs.insert(zaddr);
-                }
-            } else {
+            if (!IsValidPaymentAddress(zaddr)) {
                 throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, address is not a valid zaddr: ") + address);
             }
+            auto hasSpendingKey = boost::apply_visitor(HaveSpendingKeyForPaymentAddress(pwalletMain), zaddr);
+            if (!fIncludeWatchonly && !hasSpendingKey) {
+                throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, spending key for address does not belong to wallet: ") + address);
+            }
+            zaddrs.insert(zaddr);
 
             if (setAddress.count(address)) {
                 throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, duplicated address: ") + address);