[BUG] Fix crash when sending shield notes from the GUI with coin control

src/sapling/saplingscriptpubkeyman.cpp

@@ -397,31 +397,25 @@ void SaplingScriptPubKeyMan::GetNotes(const std::vector<SaplingOutPoint>& saplin
     for (const auto& outpoint : saplingOutpoints) {
         const auto* wtx = wallet->GetWalletTx(outpoint.hash);
         if (!wtx) throw std::runtime_error("No transaction available for hash " + outpoint.hash.GetHex());
+        const int depth = WITH_LOCK(wallet->cs_wallet, return wtx->GetDepthInMainChain(); );
         const auto& it = wtx->mapSaplingNoteData.find(outpoint);
         if (it != wtx->mapSaplingNoteData.end()) {
-
             const SaplingOutPoint& op = it->first;
             const SaplingNoteData& nd = it->second;
 
             // skip sent notes
             if (!nd.IsMyNote()) continue;
-            const libzcash::SaplingIncomingViewingKey& ivk = *(nd.ivk);
-
-            const OutputDescription& outDesc = wtx->tx->sapData->vShieldedOutput[op.n];
-            auto maybe_pt = libzcash::SaplingNotePlaintext::decrypt(
-                    outDesc.encCiphertext,
-                    ivk,
-                    outDesc.ephemeralKey,
-                    outDesc.cmu);
-            assert(static_cast<bool>(maybe_pt));
-            auto notePt = maybe_pt.get();
 
-            auto maybe_pa = ivk.address(notePt.d);
-            assert(static_cast<bool>(maybe_pa));
-            auto pa = maybe_pa.get();
+            // recover plaintext and address
+            auto optNotePtAndAddress = wtx->DecryptSaplingNote(op);
+            assert(static_cast<bool>(optNotePtAndAddress));
 
+            const libzcash::SaplingIncomingViewingKey& ivk = *(nd.ivk);
+            const libzcash::SaplingNotePlaintext& notePt = optNotePtAndAddress->first;
+            const libzcash::SaplingPaymentAddress& pa = optNotePtAndAddress->second;
             auto note = notePt.note(ivk).get();
-            saplingEntriesRet.emplace_back(op, pa, note, notePt.memo(), wtx->GetDepthInMainChain());
+
+            saplingEntriesRet.emplace_back(op, pa, note, notePt.memo(), depth);
         }
     }
 }
@@ -471,31 +465,27 @@ void SaplingScriptPubKeyMan::GetFilteredNotes(
         }
 
         // Filter the transactions before checking for notes
+        const int depth = wtx.GetDepthInMainChain();
         if (!IsFinalTx(wtx.tx, wallet->GetLastBlockHeight() + 1, GetAdjustedTime()) ||
-            wtx.GetDepthInMainChain() < minDepth ||
-            wtx.GetDepthInMainChain() > maxDepth) {
+            depth < minDepth || depth > maxDepth) {
             continue;
         }
 
         for (const auto& it : wtx.mapSaplingNoteData) {
             const SaplingOutPoint& op = it.first;
             const SaplingNoteData& nd = it.second;
 
-            // Skip sent notes
+            // skip sent notes
             if (!nd.IsMyNote()) continue;
-            const libzcash::SaplingIncomingViewingKey& ivk = *(nd.ivk);
 
-            auto maybe_pt = libzcash::SaplingNotePlaintext::decrypt(
-                    wtx.tx->sapData->vShieldedOutput[op.n].encCiphertext,
-                    ivk,
-                    wtx.tx->sapData->vShieldedOutput[op.n].ephemeralKey,
-                    wtx.tx->sapData->vShieldedOutput[op.n].cmu);
-            assert(static_cast<bool>(maybe_pt));
-            auto notePt = maybe_pt.get();
+            // recover plaintext and address
+            auto optNotePtAndAddress = wtx.DecryptSaplingNote(op);
+            assert(static_cast<bool>(optNotePtAndAddress));
 
-            auto maybe_pa = ivk.address(notePt.d);
-            assert(static_cast<bool>(maybe_pa));
-            auto pa = maybe_pa.get();
+            const libzcash::SaplingIncomingViewingKey& ivk = *(nd.ivk);
+            const libzcash::SaplingNotePlaintext& notePt = optNotePtAndAddress->first;
+            const libzcash::SaplingPaymentAddress& pa = optNotePtAndAddress->second;
+            auto note = notePt.note(ivk).get();
 
             // skip notes which belong to a different payment address in the wallet
             if (!(filterAddresses.empty() || filterAddresses.count(pa))) {
@@ -516,8 +506,7 @@ void SaplingScriptPubKeyMan::GetFilteredNotes(
             //    continue;
             //}
 
-            auto note = notePt.note(ivk).get();
-            saplingEntries.emplace_back(op, pa, note, notePt.memo(), wtx.GetDepthInMainChain());
+            saplingEntries.emplace_back(op, pa, note, notePt.memo(), depth);
         }
     }
 }

src/test/librust/sapling_wallet_tests.cpp

@@ -1147,6 +1147,102 @@ BOOST_AUTO_TEST_CASE(MarkAffectedSaplingTransactionsDirty) {
     RegtestDeactivateSapling();
 }
 
+BOOST_AUTO_TEST_CASE(GetNotes)
+{
+    auto consensusParams = RegtestActivateSapling();
+
+    CWallet& wallet = *pwalletMain;
+    libzcash::SaplingPaymentAddress pk;
+    uint256 blockHash;
+    std::vector<SaplingOutPoint> saplingOutpoints;
+    {
+        LOCK2(cs_main, wallet.cs_wallet);
+        setupWallet(wallet);
+
+        // Generate Sapling address
+        auto sk = GetTestMasterSaplingSpendingKey();
+        auto extfvk = sk.ToXFVK();
+        pk = sk.DefaultAddress();
+
+        BOOST_CHECK(wallet.AddSaplingZKey(sk));
+        BOOST_CHECK(wallet.HaveSaplingSpendingKey(extfvk));
+
+        // Set up transparent address
+        CBasicKeyStore keystore;
+        CKey tsk = AddTestCKeyToKeyStore(keystore);
+        auto scriptPubKey = GetScriptForDestination(tsk.GetPubKey().GetID());
+
+        // Generate shielding tx from transparent to Sapling (five 1 PIV notes)
+        auto builder = TransactionBuilder(consensusParams, 1, &keystore);
+        builder.AddTransparentInput(COutPoint(), scriptPubKey, 510000000);
+        for (int i=0; i<5; i++) builder.AddSaplingOutput(extfvk.fvk.ovk, pk, 100000000, {});
+        builder.SetFee(10000000);
+        auto tx1 = builder.Build().GetTxOrThrow();
+
+        CWalletTx wtx {&wallet, MakeTransactionRef(tx1)};
+
+        // Fake-mine the transaction
+        BOOST_CHECK_EQUAL(0, chainActive.Height());
+        SaplingMerkleTree saplingTree;
+        CBlock block;
+        block.vtx.emplace_back(wtx.tx);
+        block.hashMerkleRoot = BlockMerkleRoot(block);
+        blockHash = block.GetHash();
+        CBlockIndex fakeIndex {block};
+        BlockMap::iterator mi = mapBlockIndex.emplace(blockHash, &fakeIndex).first;
+        fakeIndex.phashBlock = &((*mi).first);
+        chainActive.SetTip(&fakeIndex);
+        BOOST_CHECK(chainActive.Contains(&fakeIndex));
+        BOOST_CHECK_EQUAL(0, chainActive.Height());
+
+        // Simulate SyncTransaction which calls AddToWalletIfInvolvingMe
+        auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first;
+        BOOST_CHECK(saplingNoteData.size() > 0);
+        wtx.SetSaplingNoteData(saplingNoteData);
+        wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0);
+        wallet.LoadToWallet(wtx);
+
+        // Simulate receiving new block and ChainTip signal
+        wallet.IncrementNoteWitnesses(&fakeIndex, &block, saplingTree);
+        wallet.GetSaplingScriptPubKeyMan()->UpdateSaplingNullifierNoteMapForBlock(&block);
+        wallet.SetLastBlockProcessed(mi->second);
+
+        const uint256& txid = wtx.GetHash();
+        for (int i=0; i<5; i++) saplingOutpoints.emplace_back(txid, i);
+    }
+
+    // Check GetFilteredNotes
+    std::vector<SaplingNoteEntry> entries;
+    Optional<libzcash::SaplingPaymentAddress> address = pk;
+    wallet.GetSaplingScriptPubKeyMan()->GetFilteredNotes(entries, address, 0, true, false);
+    for (int i=0; i<5; i++) {
+        BOOST_CHECK(entries[i].op == saplingOutpoints[i]);
+        BOOST_CHECK(entries[i].address == pk);
+        BOOST_CHECK_EQUAL(entries[i].confirmations, 1);
+    }
+
+    // Check GetNotes
+    std::vector<SaplingNoteEntry> entries2;
+    wallet.GetSaplingScriptPubKeyMan()->GetNotes(saplingOutpoints, entries2);
+    for (int i=0; i<5; i++) {
+        BOOST_CHECK(entries2[i].op == entries[i].op);
+        BOOST_CHECK(entries2[i].address == entries[i].address);
+        BOOST_CHECK(entries2[i].note.d == entries[i].note.d);
+        BOOST_CHECK_EQUAL(entries2[i].note.pk_d, entries[i].note.pk_d);
+        BOOST_CHECK_EQUAL(entries2[i].note.r, entries[i].note.r);
+        BOOST_CHECK(entries2[i].memo == entries[i].memo);
+        BOOST_CHECK_EQUAL(entries2[i].confirmations, entries[i].confirmations);
+    }
+
+    // Tear down
+    LOCK(cs_main);
+    chainActive.SetTip(nullptr);
+    mapBlockIndex.erase(blockHash);
+
+    // Revert to default
+    RegtestDeactivateSapling();
+}
+
 // TODO: Back port WriteWitnessCache & SetBestChainIgnoresTxsWithoutShieldedData test cases.
 
 BOOST_AUTO_TEST_SUITE_END()

src/wallet/wallet.cpp

@@ -4755,7 +4755,7 @@ void CWalletTx::SetSaplingNoteData(mapSaplingNoteData_t &noteData)
 
 Optional<std::pair<
         libzcash::SaplingNotePlaintext,
-        libzcash::SaplingPaymentAddress>> CWalletTx::DecryptSaplingNote(SaplingOutPoint op) const
+        libzcash::SaplingPaymentAddress>> CWalletTx::DecryptSaplingNote(const SaplingOutPoint& op) const
 {
     // Check whether we can decrypt this SaplingOutPoint with the ivk
     auto it = this->mapSaplingNoteData.find(op);
@@ -4783,8 +4783,7 @@ Optional<std::pair<
 
 Optional<std::pair<
         libzcash::SaplingNotePlaintext,
-        libzcash::SaplingPaymentAddress>> CWalletTx::RecoverSaplingNote(
-        SaplingOutPoint op, std::set<uint256>& ovks) const
+        libzcash::SaplingPaymentAddress>> CWalletTx::RecoverSaplingNote(const SaplingOutPoint& op, const std::set<uint256>& ovks) const
 {
     auto output = this->tx->sapData->vShieldedOutput[op.n];
 

src/wallet/wallet.h

@@ -452,16 +452,15 @@ class CWalletTx
 
     void BindWallet(CWallet* pwalletIn);
 
-    void SetSaplingNoteData(mapSaplingNoteData_t &noteData);
+    void SetSaplingNoteData(mapSaplingNoteData_t& noteData);
 
     Optional<std::pair<
             libzcash::SaplingNotePlaintext,
-            libzcash::SaplingPaymentAddress>> DecryptSaplingNote(SaplingOutPoint op) const;
+            libzcash::SaplingPaymentAddress>> DecryptSaplingNote(const SaplingOutPoint& op) const;
 
     Optional<std::pair<
             libzcash::SaplingNotePlaintext,
-            libzcash::SaplingPaymentAddress>> RecoverSaplingNote(
-            SaplingOutPoint op, std::set<uint256>& ovks) const;
+            libzcash::SaplingPaymentAddress>> RecoverSaplingNote(const SaplingOutPoint& op, const std::set<uint256>& ovks) const;
 
     //! checks whether a tx has P2CS inputs or not
     bool HasP2CSInputs() const;